Skip to content
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
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

javierm
Copy link
Member

@javierm javierm commented May 6, 2024

References

Objectives

  • Reduce code duplication in database configuration files
  • Avoid having to add yet another database configuration file or change the default one in pull request Codespace #5100

Notes

TODO: Check whether we could omit the environment variables in the database.yml file and use PGUSER, PGHOST and PGPASSWORD in Docker.

Documentation

We need to change the docker database file references and replace them with the regular .example file.

@javierm javierm self-assigned this May 6, 2024
@javierm javierm added this to Reviewing in Consul Democracy May 6, 2024
@javierm javierm mentioned this pull request May 6, 2024
@javierm javierm force-pushed the unify_database_config_files branch from 10813a4 to ff60872 Compare May 6, 2024 02:42
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 javierm force-pushed the unify_database_config_files branch 3 times, most recently from 819bcb5 to 5918ae5 Compare May 6, 2024 13:12
@javierm javierm moved this from Reviewing to Doing in Consul Democracy May 6, 2024
@javierm javierm force-pushed the unify_database_config_files branch 3 times, most recently from 818e2a6 to 2d6fabb Compare May 6, 2024 20:35
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 javierm force-pushed the unify_database_config_files branch from 2d6fabb to 2d70104 Compare May 6, 2024 20:38
@javierm javierm moved this from Doing to Reviewing in Consul Democracy May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Consul Democracy
  
Reviewing
Development

Successfully merging this pull request may close these issues.

None yet

1 participant