Improve performance by caching Filament components and warn if APP_KEY is missing #19
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for opening this pull request! Be sure to follow the pull request template!
|
I am a bot, here are the test results for this PR:
|
|
If we're going to make that change then you also need to update the readme-vars to move the APP_KEY env from optional to mandatory and add a note to the changelog so we've got an easy record of when the change was made. |
|
On it, @thespad just to confirm you'll want |
|
Just the readme-vars, the actual readme.md is templated at build time. |
|
@thespad pushed the updates, let me know if you need anything else |
|
I am a bot, here are the test results for this PR:
|
|
So based on my testing of 0.20, Without it set. We can set it to a UTC default in the Dockerfiles if that's easier. Also your example settings for - SPEEDTEST_SCHEDULE=47 */6 * * *
- SPEEDTEST_SERVERS=60700,51643,57802,22971or - "SPEEDTEST_SCHEDULE=47 */6 * * *"
- "SPEEDTEST_SERVERS=60700,51643,57802,22971"or even SPEEDTEST_SCHEDULE: 47 */6 * * *
SPEEDTEST_SERVERS: 60700,51643,57802,22971but not - SPEEDTEST_SCHEDULE="47 */6 * * *"
- SPEEDTEST_SERVERS="60700,51643,57802,22971"As that will include the quotes as part of the env (unless you have code in place to handle that internally). |
|
Oh, also you might want to change |
Good catch, it does sound like one is required. I should be defaulting to Also examples have been updated, thanks for the suggestion. I wrote them looking at my |
|
I am a bot, here are the test results for this PR:
|
|
Seeing Should we be copying env.example instead? |
I'm no longer using I can remove this copy statement in a few unless I'm missing an issue this would cause. I do believe I have coverage across vars with |
|
You need to also change this if ! grep -E "APP_KEY=[0-9A-Za-z:+\/=]{1,}" /app/www/.env > /dev/null; thenProbably to something like if ! grep -qE "APP_KEY=[0-9A-Za-z:+\/=]{1,}" /app/www/.env 2> /dev/null; thenSo that it will handle people who already have an APP_KEY set in their .env file but won't error if there isn't one at all. |
|
I also moved creating the symlink to only if an |
|
I am a bot, here are the test results for this PR:
|
|
I think we need to do a |
|
Realised it was quicker just to do those bits myself - added an app_key to the CI envs so it boots correctly for the CI tests. |
|
Though obviously that's still going to fail right now because of the DISPLAY_TIMEZONE bug. |
|
Cool thank you, I've been trying to respond between meetings this morning.
|
|
Oh, I forgot to ask, what's the behaviour for an existing user updating to 0.20 who doesn't set SPEEDTEST_SCHEDULE or SPEEDTEST_SERVERS? |
|
Their scheduled tests will stop until they get those options setup. |
|
In that case I'd suggest some kind of UI indication that there's no schedule and/or server selection or we're going to get a bunch of users who don't understand why their installs have stopped working. |
|
Oh and don't worry about the CI tests failing all of a sudden, I forgot that external PRs don't use the generated Jenkinsfile for security reasons, so the sleep is taking effect and not the APP_KEY env, but it'll be fine once merged. |
Good idea, captured that separately so I don't lose it in comments. |
|
Updated the readme with the new envs, I think we're probably good to merge this unless you've got anything else you think needs to be updated? |
I'm good, any issues should be application side which I'll handle in app releases. Thanks! |
Description:
This PR improves performance by caching Filament components during startup and will now warn a user if an
APP_KEYis not present and how to generate one.Benefits of this PR and context:
Improving performance by caching views and documentation by guiding user setup.
How Has This Been Tested?
Same startup process for caching I'm using in
app:installcommand for development.Source / References:
https://github.com/alexjustesen/speedtest-tracker/blob/ce697bd9aae51227130ef97d2ec80647a3a6e134/app/Console/Commands/InstallCommand.php#L41-L43