-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Unify database configuration files #5530
Open
javierm
wants to merge
3
commits into
master
Choose a base branch
from
unify_database_config_files
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Open
javierm
force-pushed
the
unify_database_config_files
branch
from
May 6, 2024 02:42
10813a4
to
ff60872
Compare
Not sure whether it was needed when we included it, but everything is working fine without it.
We're using version 13 because it's the one included in Debian Bullseye, which is the operating system we currently use in our Dockerfile. For consistency, we're using the same version in GitHub Actions. Note this image requires setting a password. Otherwise we get an error: > Database is uninitialized and superuser password is not specified. > You must specify POSTGRES_PASSWORD to a non-empty value for the > superuser. For example, "-e POSTGRES_PASSWORD=password" on > "docker run". Since now we're setting a password in the postgres service, we also need to provide the `PGPASSWORD` environment variable. Otherwise we get an error: ``` PG::ConnectionBad: connection to server at "::1", port 5432 failed: fe_sendauth: no password supplied (PG::ConnectionBad) ```
javierm
force-pushed
the
unify_database_config_files
branch
3 times, most recently
from
May 6, 2024 13:12
819bcb5
to
5918ae5
Compare
javierm
force-pushed
the
unify_database_config_files
branch
3 times, most recently
from
May 6, 2024 20:35
818e2a6
to
2d6fabb
Compare
We had three files that were almost identical, and we can use environment variables to specify the differences. Note we're using the `PGUSER` and `PGPASSWORD` variables, since these variables will automatically be used by the PostgreSQL client when we have a blank `username` and `password` keys in the `database.yml` file (which we did until now). The difference between these variables and the `POSTGRES_USER` and `POSTGRES_PASSWORD` variables is that the `PG` variables are used by the client connecting to the database, while the `POSTGRES_` variables are used by the Docker postgresql image when creating the database superuser. For consistency with the code in our github workflows (and everywhere else in the postgres world), we're respecting this double standard. The fact that there are two different names for what's basically the same thing makes the code confusing, though, particularly when running the docker-compose commands, since we get the password from an environment variable but we have to assign two different environment variables with it. So we're accepting both `PGPASSWORD` and `POSTGRES_PASSWORD` variables in the database configuration file. This way, developers using docker-compose can use `POSTGRES_PASSWORD` for everything and it'll work fine. Also note we're using `DB_HOST` instead of `PGHOST` because that's the variable Rails currently uses by default for new applications [1]. Finally, note we're using `.presence` in the `ENV` calls in the database.yml file. The `PGPASSWORD` variable was set to an empty string when running docker-compose, so using `ENV["PGPASSWORD"] ||` wouldn't work. [1] https://github.com/rails/rails/blob/c90a8701e5/railties/lib/rails/generators/rails/app/templates/config/databases/postgresql.yml.tt#L22
javierm
force-pushed
the
unify_database_config_files
branch
from
May 6, 2024 20:38
2d6fabb
to
2d70104
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Notes
TODO: Check whether we could omit the environment variables in the
database.yml
file and usePGUSER
,PGHOST
andPGPASSWORD
in Docker.Documentation
We need to change the docker database file references and replace them with the regular
.example
file.