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

Add a config option for the postgres database port #4433

Merged
merged 2 commits into from
May 17, 2024

Conversation

davidmehren
Copy link
Contributor

This is a continuation of #3631, with most comments addressed.

The last open point of discussion was whether it makes sense to add the POSTGRES_PORT config option to the AiO Postgres container. I don't think so, and wrote:

I didn't add the option for the AIO Postgres container, as it doesn't really make sense to change the port when the internal database is used. The new option is only relevant when an external database is used.

This PR has been tested using these steps:

  • build the nextcloud and notify-push containers manually
  • create a database and user in the external Postgres DB (both must be prefixed with oc_, as that is hardcoded at various places)
  • follow the instructions for manual installation and edit the compose.yml
    • remove the nextcloud-aio-database container and the dependency from nextcloud-aio-nextcloud to it
    • replace the images of the nextcloud and notify-push containers with the local builds
    • set POSTGRES_HOST, POSTGRES_PORT, POSTGRES_HOST and POSTGRES_DB environment variables for the nextcloud and notify-push containers. Note that POSTGRES_USER must not include the oc_ prefix, while POSTGRES_DB needs to contain it.
  • docker compose up should then set up a new Nextcloud using the external database

@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature or request labels Mar 25, 2024
@szaimen szaimen added this to the next milestone Mar 25, 2024
@szaimen
Copy link
Collaborator

szaimen commented Mar 26, 2024

Hi, thanks for your PR!

Please address the failing tests and I still find occurences of 5432 in this container: https://github.com/nextcloud/all-in-one/tree/main/Containers/postgresql that should also be adjusted since otherwise people might be confused if they change this but the default bundled container does not handle this correctly

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 26, 2024
@davidmehren davidmehren force-pushed the postgres_port branch 5 times, most recently from cf0ff4d to 1fbfebb Compare March 26, 2024 19:27
@davidmehren
Copy link
Contributor Author

The port of the Postgres container is now also configurable.

The shellcheck errors were false positives and are now disabled with an explanatory comment.

@szaimen szaimen modified the milestones: v8.1.0, next Mar 28, 2024
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 3, 2024
@szaimen szaimen modified the milestones: v8.2.0, next Apr 16, 2024
@szaimen szaimen modified the milestones: v8.2.1, next Apr 26, 2024
davidmehren and others added 2 commits May 17, 2024 14:05
Signed-off-by: David Mehren <git@herrmehren.de>
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen
Copy link
Collaborator

szaimen commented May 17, 2024

Thanks for the PR and sorry that it took so long to review this!

I reverted the change to the database container in b1ad854 since I came to the conclusion that it is easier to maintain if the database container is not adjustable.

@szaimen szaimen merged commit de5aeb6 into nextcloud:main May 17, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants