-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 POSTGRES_PASSWORD and USER, with warning on no password #36
Conversation
@@ -9,8 +9,29 @@ if [ "$1" = 'postgres' ]; then | |||
|
|||
sed -ri "s/^#(listen_addresses\s*=\s*)\S+/\1'*'/" "$PGDATA"/postgresql.conf | |||
|
|||
{ echo; echo 'host all all 0.0.0.0/0 trust'; } >> "$PGDATA"/pg_hba.conf | |||
|
|||
TYPE='CREATE' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lowercase/camel for "local" vars please. 👍 (and in that case, type
is already an instruction so you'll need to choose a new word like op
or something 😄)
Approach seems sane to me, just mostly minor style nits! 👍 |
Won't this fail in the case that |
Ahahaha, good catch @md5. |
👐 |
|
||
TYPE='CREATE' | ||
: ${POSTGRES_USER:=postgres} | ||
if [ "$POSTGRES_USER" = postgres ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add quotes here? It's more symmetrical
if [ "$POSTGRES_USER" = "postgres" ]; then
Fixed many nits; fixed specifying user and no pass. |
|
||
# check password first so we can ouptut the warning before postgres | ||
# messes it up | ||
pass= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent, shouldn't this be down in the else
with authMethod=trust
?
LGTM other than that one minor consistency nit |
LGTM |
Add POSTGRES_PASSWORD and USER, with warning on no password
Fixed! (and a sneak version bump to 9.4 for rc1) |
Nice. Do you think this will be up in the Registry today? I'll build a new version of my |
👍, I am working on the docs for it and I'll poke @tianon to make the PR for official-images. |
This caused a small issue for us. We were using a specific user to connect to the database, and that user was blocked by this change. The solution we opted for (as this is a development environment) was to use a single user for everything, but in production we of course do things differently. |
You can also create additional users and databases during container initialization (in custom script). |
This also broke our dev environment, we have a couple different users in our setup to mirror prod and now it seems to be not possible to use this postgres container with multiple remote users. Or is there a way I'm not thinking of? Can you make it an option to use |
Indeed. Also, |
@mmarzantowicz sorry not sure I understand, we do create user accounts with passwords but we have multiple user accounts and it looks like the entrypoint script will only allow access by 1 user via the Otherwise we can do as suggested using the hook in PR #23 |
Makes sense, thanks for the replies. |
Fixes #31