-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Refactor env vars from file #1942
Refactor env vars from file #1942
Conversation
@@ -1,38 +1,26 @@ | |||
<?php | |||
|
|||
require_once "util.php"; |
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.
Not familiar with the autoloading for this. Feedback welcome
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.
Furthermore, Nextcloud accepts some parameters from NC_
prefixed environment variables. This behaviour is already complex enough. I'd not add any custom code inclusions in the config directory.
The reason why all database parameters should be configured is because the automated setup only works if all of them are specified:
Line 177 in 405e815
elif [ -n "${POSTGRES_DB+x}" ] && [ -n "${POSTGRES_USER+x}" ] && [ -n "${POSTGRES_PASSWORD+x}" ] && [ -n "${POSTGRES_HOST+x}" ]; then |
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.
As for the first part, are you suggesting that I try to have getFileEnv
included by renaming the file to x.config.php? Or simply forget the whole idea?
As for the database part -
In the entrypoint script those values can independently be from file or not.
Lines 162 to 164 in 405e815
file_env POSTGRES_DB | |
file_env POSTGRES_PASSWORD | |
file_env POSTGRES_USER |
The improvement here isn't that we allow some to go missing, it is that this PR just so happens to allow the same thing in the PHP. That is,
current behavior (as described in the README section removed by this PR)
- must have postgres_db, _user, _password, _host OR
- postgres_db_file, _user_file, _password_file, _host
updated behavior
- must have one of postgres_db or postgres_db_file AND
- _user or user_file AND
- _password or _password_file AND
- _host
Just noting that any potential confusion from this (e.g. documented by #1148 (comment)) is additionally avoided by this PR.
Add utility function for environment variable / from file support. Signed-off-by: Anderson Entwistle <46688047+aentwist@users.noreply.github.com>
This PR depends on all 'environment variable from file' changes. What is included in it vs. what is included in other open pull requests depends on the order in which they are merged. I suggest merging the feature branches first, and then making changes to this refactor pull request as needed. I would be happy to bring this up-to-date after the other PRs are merged.
Note that as a result of this pull request, the database configuration environment variables will become independent. That is, for example,
POSTGRES_USER
,POSTGRES_PASSWORD
, andPOSTGRES_DB
may be defined to be from file or not independently. No longer will there be confusion over defining some but not all from file.Related to #1938, #1945
Closes #1940