Add env var DATABASE_INSTALL to skip PostgreSQL install #237
Conversation
This file contains hidden or 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
The previous version breaks if DATABASE_USERNAME is unset/empty.
Previously we used `psql --version`, which actually checks the client utility rather than the server. Switch to checking whether the `postgres` user exists, since that's a simple and fairly reliable test. This commit drops the version check, because it's fairly tricky to check the server version (and we don't have much control over the version anyway because we are installing the package from the archives). Some ideas that were discarded: - `postgres --version`: Fails because postgres is not in $PATH. - `pg_config --version`: This is supposed to print the server version, but it succeeds even if the server is not installed (e.g. when the libpq-dev package is installed). - `sudo -u postgres psql -t postgres -c 'SELECT version();'`: If the server is installed but not running, this fails. But in that case we probably shouldn't try to install the server.
If the database is running on another host/container, then we don't need to install the PostgreSQL server on the web server. This is useful for CI, and mirrors the existing REDIS_INSTALL environment variable. We always need the PostgreSQL client utilities, so install them separately. If we skip installation of the database then we don't try to create the database user, because that's unlikely to work with remote databases.
We should have a working database user, so we shouldn't need sudo (and sudo wouldn't help if the database is remote). Aside: The syntax `cmd=(...)` creates a bash array, it is required to properly handle empty strings and avoid other quoting issues.
Holmes98
approved these changes
Jan 2, 2024
Member
Holmes98
left a comment
Holmes98
left a comment
There was a problem hiding this comment.
Works with the default configuration. Doesn't work with a non-default DATABASE_USERNAME (using peer authentication), but that didn't work before anyway, it just fails earlier now.
tom93
added a commit
that referenced
this pull request
Jan 2, 2024
tom93
added a commit
that referenced
this pull request
Jan 2, 2024
tom93
added a commit
that referenced
this pull request
Jan 2, 2024
The previous version breaks if DATABASE_USERNAME is unset/empty.
tom93
added a commit
that referenced
this pull request
Jan 2, 2024
Previously we used `psql --version`, which actually checks the client utility rather than the server. Switch to checking whether the `postgres` user exists, since that's a simple and fairly reliable test. This commit drops the version check, because it's fairly tricky to check the server version (and we don't have much control over the version anyway because we are installing the package from the archives). Some ideas that were discarded: - `postgres --version`: Fails because postgres is not in $PATH. - `pg_config --version`: This is supposed to print the server version, but it succeeds even if the server is not installed (e.g. when the libpq-dev package is installed). - `sudo -u postgres psql -t postgres -c 'SELECT version();'`: If the server is installed but not running, this fails. But in that case we probably shouldn't try to install the server.
tom93
added a commit
that referenced
this pull request
Jan 2, 2024
If the database is running on another host/container, then we don't need to install the PostgreSQL server on the web server. This is useful for CI, and mirrors the existing REDIS_INSTALL environment variable. We always need the PostgreSQL client utilities, so install them separately. If we skip installation of the database then we don't try to create the database user, because that's unlikely to work with remote databases.
Contributor
Author
|
(Deployed the trivial change to config/database.yml manually.) |
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.
And a lot of cleanup. Came up from #223.
(The GitHub runner already has the postgresql package installed, so this won't save an install in the current setup, but it may save an install in other environments.)
Comments:
I don't know what config/database.yml.default is for (has been there since first commit, appears unused). We should eventually get rid of the install process that generates database.yml from the template, and at that point there would just be a database.yml that works out-of-the-box -- no need for a sample.
Merge using "rebase".