Skip to content

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

Merged
merged 7 commits into from
Feb 23, 2015
Merged

Added POSTGRES_DB variable to optionally specify DB name #49

merged 7 commits into from
Feb 23, 2015

Conversation

rothgar
Copy link
Contributor

@rothgar rothgar commented Feb 13, 2015

POSTGRESS_DB defaults to POSTGRESS_USER if it's not specified.

Example run

docker run -d --name postgres -e POSTGRES_DB=testdb -e POSTGRES_USER=postgres -e POSTGRES_PASSWORD=password postgres

You can also connect to the db with

docker run --rm -it --name dbconnect --link postgres:db postgres sh -c 'exec psql -h "$DB_PORT_5432_TCP_ADDR" -p "$DB_PORT_5432_TCP_PORT" -d testdb -U postgres'

@yosifkit
Copy link
Member

It seems that all but 8.4 got spaces instead of tabs on the : ${POSTGRES_DB:=$POSTGRES_USER} line; they should all be tabs like the rest of the lines. Other than that, this seems solid.

@rothgar
Copy link
Contributor Author

rothgar commented Feb 13, 2015

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.

@yosifkit
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed one. 😉

@tianon
Copy link
Member

tianon commented Feb 13, 2015

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.

@rothgar
Copy link
Contributor Author

rothgar commented Feb 13, 2015

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.

@yosifkit
Copy link
Member

Looks like the template is missing the change on the CREATE DATABASE line.

@tianon I don't see why the user creation/update would need to change. It will still have access to whatever database is created.

@rothgar
Copy link
Contributor Author

rothgar commented Feb 13, 2015

@yosifkit fixed the template. Sorry, was doing that in a rush and didn't check everything.

@yosifkit
Copy link
Member

It's all good. LGTM

ping @tianon

@tianon
Copy link
Member

tianon commented Feb 14, 2015

Can you confirm that all the following combinations work?

  • POSTGRES_USER=postgres, POSTGRES_DB=postgres
  • POSTGRES_USER=postgres, POSTGRES_DB=testdb
  • POSTGRES_USER=testuser, POSTGRES_DB=postgres
  • POSTGRES_USER=testuser, POSTGRES_DB=testdb

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.

@md5
Copy link
Contributor

md5 commented Feb 14, 2015 via email

@rothgar
Copy link
Contributor Author

rothgar commented Feb 14, 2015

I had to add another check to make sure the user was altered or created properly, but now all of these check out.

  • ✔️POSTGRES_USER=postgres, POSTGRES_DB=postgres
  • ✔️POSTGRES_USER=postgres, POSTGRES_DB=testdb
  • ✔️POSTGRES_USER=testuser, POSTGRES_DB=postgres
  • ✔️POSTGRES_USER=testuser, POSTGRES_DB=testdb
  • ✔️POSTGRES_DB=testdb

It might not be ideal that the postgres database still exists no matter if you specify POSTGRES_DB to be something different or not. I could add DROP DATABASE IF EXISTS postgres if the user specifies a database but I'm not sure that would matter.

@yosifkit
Copy link
Member

LGTM

op='CREATE'
: ${POSTGRES_DB:=$POSTGRES_USER}

if [ ! "$POSTGRES_DB" = 'postgres' ]; then
Copy link
Member

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 ! ... =? 😇

Copy link
Contributor Author

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.

@md5
Copy link
Contributor

md5 commented Feb 20, 2015

LGTM

yosifkit added a commit that referenced this pull request Feb 23, 2015
Added POSTGRES_DB variable to optionally specify DB name
@yosifkit yosifkit merged commit ebecea2 into docker-library:master Feb 23, 2015
@rothgar rothgar deleted the postgres_db_var branch February 23, 2015 23:51
tianon added a commit to infosiftr/stackbrew that referenced this pull request Mar 3, 2015
- `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
@maslick
Copy link

maslick commented Nov 28, 2015

Is it possible to create multiple databases at startup?

docker-compose.yml

db:
    image: postgres
    environment:
        - POSTGRES_DB=db1
        - POSTGRES_DB=db2
        - POSTGRES_USER=user
        - POSTGRES_PASSWORD=pwd
        - POSTGRES_ROOT_PASSWORD=rootpwd

@yosifkit
Copy link
Member

yosifkit commented Jan 5, 2016

@maslickm, not with this environment variable, but you could add a simple sql script into /docker-entrypoint-initdb.d/ and it will be run just after initial database/user creation.

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.

5 participants