all-in-one icon indicating copy to clipboard operation
all-in-one copied to clipboard

Add a config option for the postgres database port

Open davidmehren opened this issue 11 months ago • 2 comments

This is a continuation of https://github.com/nextcloud/all-in-one/pull/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

davidmehren avatar Mar 24 '24 10:03 davidmehren

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 avatar Mar 26 '24 13:03 szaimen

The port of the Postgres container is now also configurable.

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

davidmehren avatar Mar 26 '24 19:03 davidmehren

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

I reverted the change to the database container in https://github.com/nextcloud/all-in-one/pull/4433/commits/b1ad854de6de5282be68578fbc4c254f113070c4 since I came to the conclusion that it is easier to maintain if the database container is not adjustable.

szaimen avatar May 17 '24 12:05 szaimen