-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added POSTGRES_DB variable to optionally specify DB name #49
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
Conversation
It seems that all but 8.4 got spaces instead of tabs on the |
Sorry about that. I wasn't showing tab characters so I didn't notice. I just retabed all the files so they should have consistent formatting now. |
LGTM What do you think @tianon? |
@@ -29,12 +29,13 @@ if [ "$1" = 'postgres' ]; then | |||
fi | |||
|
|||
: ${POSTGRES_USER:=postgres} | |||
: ${POSTGRES_DB:=$POSTGRES_USER} | |||
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.
Missed one. 😉
Doesn't the logic down below this need to change too? gosu postgres postgres --single -jE <<-EOSQL
$op USER "$POSTGRES_USER" WITH SUPERUSER $pass ;
EOSQL I'm +1 in general; I think this is a reasonable change. |
I fixed the missed variable in 9.1 and the template @tianon Because there's only one db and one user I don't know if the logic needs to change. It worked in my test containers (specifying combinations of POSTGRES_USER, POSTGRES_DB, or none at all) by logging into the db with another container (see test above). But I'm not very familiar with postgres in general so there may be a better way. |
Looks like the template is missing the change on the @tianon I don't see why the user creation/update would need to change. It will still have access to whatever database is created. |
@yosifkit fixed the template. Sorry, was doing that in a rush and didn't check everything. |
It's all good. LGTM ping @tianon |
Can you confirm that all the following combinations work?
I just want to make sure we're covering our bases. The change LGTM, but I'll feel better about merging if you can confirm you've tested all four possible permutations of these two interacting before we merge. |
It would be awesome to see those in the tests in official-images :)
|
I had to add another check to make sure the user was altered or created properly, but now all of these check out.
It might not be ideal that the postgres database still exists no matter if you specify |
LGTM |
op='CREATE' | ||
: ${POSTGRES_DB:=$POSTGRES_USER} | ||
|
||
if [ ! "$POSTGRES_DB" = '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 make this !=
instead of ! ... =
? 😇
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.
That makes sense, I'll change it.
LGTM |
Added POSTGRES_DB variable to optionally specify DB name
- `django`: 1.7.5, Python 3.4.3 - `ghost`: 0.5.9 - `golang`: change `/go` to be `777` (docker-library/golang#44) - `java`: 7u75-2.5.4-2 - `mariadb`: 10.1.3, 10.0.17 - `mongo`: 3.0.0 - `php`: more aggressive autoremove (docker-library/php#71) - `postgres`: `POSTGRES_DB` (docker-library/postgres#49) - `python`: 3.4.3, `--enable-unicode=ucs4` (docker-library/python#38) - `rails`: Ruby 2.2.1 - `ruby`: 2.2.1, 2.0.0-p643
Is it possible to create multiple databases at startup? docker-compose.yml
|
@maslickm, not with this environment variable, but you could add a simple sql script into |
POSTGRESS_DB defaults to POSTGRESS_USER if it's not specified.
Example run
You can also connect to the db with