-
Notifications
You must be signed in to change notification settings - Fork 174
Fix for rails getting confused by an empty env var #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
docker-entrypoint.sh
Outdated
| # https://github.com/docker-library/redmine/issues/397 | ||
| # empty string is truthy in ruby and so masks the generated fallback config | ||
| # https://github.com/rails/rails/blob/1aa9987169213ce5ce43c20b2643bc64c235e792/railties/lib/rails/application.rb#L454 | ||
| export SECRET_KEY_BASE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think the bug is really closer to missing the unset SECRET_KEY_BASE below when it's empty and we already have a secret_token.rb, right? Maybe we should be more explicit about that half instead of the export half?
|
I pushed another commit, but I'm not convinced I actually like it - the original had simpler logic. |
docker-entrypoint.sh
Outdated
| fi | ||
| # generate SECRET_KEY_BASE if not set; this is not recommended unless the secret_token.rb is saved when container is recreated | ||
| if [ -z "$SECRET_KEY_BASE" ] && [ ! -f config/initializers/secret_token.rb ]; then | ||
| # generate SECRET_KEY_BASE in-file if not set; this is not recommended unless the secret_token.rb is saved when container is recreated | ||
| if [ -z "${SECRET_KEY_BASE:-}" ] && [ ! -f config/initializers/secret_token.rb ]; then | ||
| echo >&2 'warning: no *SECRET_KEY_BASE set; running `rake generate_secret_token` to create one in "config/initializers/secret_token.rb"' | ||
| unset SECRET_KEY_BASE # just in case | ||
| rake generate_secret_token | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like your change better but maybe we should also combine this second if to be inside the first rather than repeating the condition?
# left out indentation changes
- fi
# generate SECRET_KEY_BASE in-file if not set; this is not recommended unless the secret_token.rb is saved when container is recreated
- if [ -z "${SECRET_KEY_BASE:-}" ] && [ ! -f config/initializers/secret_token.rb ]; then
+ if [ ! -f config/initializers/secret_token.rb ]; then
echo >&2 'warning: no *SECRET_KEY_BASE set; running `rake generate_secret_token` to create one in "config/initializers/secret_token.rb"'
rake generate_secret_token
fi
+ fi
docker-entrypoint.sh
Outdated
| unset SECRET_KEY_BASE | ||
| # generate SECRET_KEY_BASE in-file if not set; this is not recommended unless the secret_token.rb is saved when container is recreated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The juxtaposition of "unset" followed by "if not set". 😆
| unset SECRET_KEY_BASE | |
| # generate SECRET_KEY_BASE in-file if not set; this is not recommended unless the secret_token.rb is saved when container is recreated | |
| unset SECRET_KEY_BASE | |
| # generate SECRET_KEY_BASE in-file since it is not set or empty; this is not recommended unless the secret_token.rb is saved when container is recreated |
yosifkit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it's as much your PR as it is mine ❤️🙈, but LGTM.
Changes: - docker-library/redmine@01d5e42: Fix for rails getting confused by an empty env var (docker-library/redmine#398)
Changes: - docker-library/redmine@01d5e42: Fix for rails getting confused by an empty env var (docker-library/redmine#398)
Fixes #397