-
Notifications
You must be signed in to change notification settings - Fork 220
Add PosgreSQL 9.2 image #1
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
9.2/run-postgresql.sh
Outdated
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.
A note about USER. We run this as root, but only thing that really requires root privileges is this chown. I would love to run the whole entrypoint as user postgres. However, we would have to make sure the volume is owned by the postgres user (uid 26).
Any ideas if this is possible?
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.
Why not chown in Dockerfile 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.
Nope if you have mounted volume $PGDATA.
e368cf4 to
a38a2c4
Compare
|
Addressed all issues plus something extra :) PTAL |
9.2/run-postgresql.sh
Outdated
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.
this needs to be tuneable via env var... why is default not enough? can't you set this in postgres config file instead?
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.
Right, will fix that tomorrow so LANG can be overridden. I might, but setting the LANG var is the simplest way to do it, I didn't see any reason to modify config file because of it. Btw, it mostly affects initdb. And yeah, the default is probably faster a bit, but without utf support, at least that's how I understand it.
|
LGTM. Nice job @mnagy, kudos! |
|
Some enhancements: |
|
Very small change added. We're now also validating maximum user/db name length. Also added tests for that. |
9.2/run-postgresql.sh
Outdated
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.
could you maybe do the function declaration in this file more consistent with the test/run script?
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.
Done. PTAL
0410f87 to
c9d7307
Compare
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 you do that also for https://github.com/openshift/mongodb/ and https://github.com/openshift/mysql/, please 😸
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.
Even more, can you add that to all our images 😅
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.
@soltysh For mysql, see sclorg/mysql-container#24
For MongoDB, @jhadvig will do that. :)
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.
many kudos @mnagy
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.
@soltysh thanks :) I'd also appreciate a certain four-letter acronym :P
|
Added SCL info into README.md, few minor fixes. PTAL |
|
Here's mine LGTM :) |
|
@mnagy LGTM, good job :) |
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.
This uses PASS and USER as global variables, so we're never actually testing the access of the admin user...
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 you open a bug report against this repo, so it doesn't get forgotten?
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.
I was wrong. In fact, what happens is that assert_login_access shadows USER and PASS with local variables of the same name, and because in bash local variables are visible in the scope of a function and its children, postgresql_cmd eventually uses those local variables when called from within assert_login_access.
Needless to say, that's very very error prone and hard to reason about...
Turn pipefail option off to not change behaviour beyond the function scope
It is based on the MySQL image and works similarly. You must set
POSTGRESQL_{USER,PASSWORD,DATABASE}environment variables and canoptionally set the 'postgres' db user password using the
POSTGRESQL_ADMIN_PASSWORDvariable.There are few minor outstanding issues that are marked and commented
with a
TODO. Ideas and suggestions would be appreciated.You can test the image with
make test && make test TARGET=rhel7.Documentation in README is still missing, but wanted early feedback.