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

set -c config_file= in docker-compose #132

Merged
merged 1 commit into from
Aug 4, 2023
Merged

Conversation

wuputah
Copy link
Member

@wuputah wuputah commented Aug 4, 2023

fixes #130

This is the recommendation per the docs ( https://hub.docker.com/_/postgres/ ) that I was following, but I missed that they had -c 'config_file=...'.

@wuputah wuputah requested a review from owenthereal August 4, 2023 19:49
@owenthereal
Copy link
Collaborator

owenthereal commented Aug 4, 2023

Is this the default behaviour for the image? If so, I think it would be better to copy the config file and the change the running command in the image:

...

COPY ./files/postgres/postgresql.conf /etc/postgresql/postgresql.conf
CMD ["postgres",  "-c", "config_file=/etc/postgresql/postgresql.conf"]

Users could further override the config & the command as they see fit via the docker flags or in docker compose.

@wuputah
Copy link
Member Author

wuputah commented Aug 4, 2023

it can [also] be done a different way, yes, but what's wrong with supporting this way? I'd like to just fix this for the moment. :)

@owenthereal
Copy link
Collaborator

owenthereal commented Aug 4, 2023

but what's wrong with supporting this way?

It depends on how users run the image. If they run it without the docker-compose file (e.g. docker run ghcr.io/hydradatabase/hydra:latest), the default command and the config won't be in affect. That's why I was trying to clarify whether using the config and command is the default behavior of running the image :). If it is, moving both to the Dockerfile would be better.

I approved it in case you want to merge it as is.

@wuputah
Copy link
Member Author

wuputah commented Aug 4, 2023

it's worthy of more thought, but let's not overthink it too much. user feedback welcome. :)

@wuputah wuputah merged commit c13e30a into main Aug 4, 2023
@wuputah wuputah deleted the jd/compose-set-config-file branch August 4, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: postgresql config file is incorrectly mapped
2 participants