-
Notifications
You must be signed in to change notification settings - Fork 27
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
Refactor startup scripts, envvars for all omero parameter #1
Conversation
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.
What's the rationale for not just calling the run-exec.sh file run.sh? That seems to be the norm.
50-config.py
Outdated
|
||
|
||
for (k, v) in os.environ.iteritems(): | ||
omero = '/opt/omero/server/OMERO.server/bin/omero' |
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.
Need not be in loop
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.
and probably should be caps
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.
Fixed
I've renamed run-exec.sh to entrypoint.sh |
Works as expected. |
README.md
Outdated
-e CONFIG_omero_db_name=postgres \ | ||
-e ROOTPASS=omero-root-password \ | ||
-p 4063:4063 -p 4064:4064 \ | ||
-e ROOTPASS=omero openmicroscopy/omero-server |
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.
ROOTPASS twice?
@@ -12,13 +12,15 @@ RUN yum -y install epel-release \ | |||
ARG OMERO_VERSION=latest |
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.
If we're using the main release version (i.e. 5.3.2
) for tags of this repo, then I'm assuming:
latest
images are considered worst-practice- on each OMERO release, we'll branch from
master
and bump this string to include the version (5.3.3
) - for bug fixes before an OMERO release, we'll suffix
5.3.3-1
Note: this doesn't yet give us a strategy for breaking changes in the image itself
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'm assuming this has everyone's implicit 👍 but now with 5.4 replacing 5.3
DBHOST="$CONFIG_omero_db_host" | ||
else | ||
DBHOST=db | ||
$omero config set omero.db.host "$DBHOST" |
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.
Does this need to be outside the if block?
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.
No, CONFIG_omero_db_host
will already be converted into omero.db.host
by 50-config.py
- I'll update the comment at the top of the file. The only reason for special handling here is that the default of omero.db.host=localhost
will never work, instead it defaults to db
which should be set by docker run --link ...
.
50-config.py
Outdated
@@ -0,0 +1,20 @@ | |||
#!/usr/bin/env python | |||
# Set omero config properties from CONFIG_ envvars | |||
# Variable names should replace "." with "_" and "_" with "__" |
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.
Not particularly worried but there's obviously some danger of a __
conflict here.
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 know. Do you have another suggestion?
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.
Not sure how evil this is but:
$ env "a.b=1" python -c "import os; print os.environ['a.b']"
1
$ docker run -ti --rm -e a.b=1 centos python -c "import os; print os.environ['a.b']"
1
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.
Unfortuntately it's also used in the bash script60-database.sh
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.
Continuing discussion under #3
README.md
Outdated
docker run -d --name postgres -e POSTGRES_PASSWORD=postgres \ | ||
-v omero-db:/var/lib/postgresql/data postgres | ||
docker run -d --name omero-server --link postgres:db | ||
-e CONFIG_omero_db_pass=dbpassword -v omero-data:/OMERO \ |
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 example isn't working for me:
FATAL: password authentication failed for user "omero"
DETAIL: Connection matched pg_hba.conf line 95: "host all all 0.0.0.0/0 md5"
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 didn't want to repeat all the -e CONFIG_omero_db_*
params from the previous example, I'll try and make that clearer
BTW, I've just realised https://github.com/openmicroscopy/omero-server-docker/blob/master/requirements.yml#L21 means this can't be tagged as production until ome/ansible-role-omero-server#12 is tagged |
oops...not sure what happened... |
I had a typo, accidently did git push ... -d. |
That's kinda scary. Good to know. |
Updated in response to @joshmoore's comments. In addition I've switched from loading One issue you may want to consider before ansible-role-omero-server is officially tagged |
Discussion now points to:
|
--depends on ome/omego#108
|
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.
If there could potentially be mounted filesystems that might want to be mounted as the same UID as the omero server, should this be made configurable? The host os is likely to have more restrictions on what uids they can use than the container one.
No objections from my side to having uid and gid be ARGs, but in general, then, you could also subclass the image and To your question in particular: I haven't run into a situation where I can't chown the directory with a particular UID before mounting but I could imagine not wanting to if that were to put something in particular at risk. (i.e. I don't trust the 1000 user on the host system in question) |
@dpwrussell I agree with you. The correct way to solve this is to treat the omero-server Docker filesystem as readonly with the exception of defined directories for logs and temp-files (which is our long-term goal) which will need to be mounted as volumes. Then you can simply use The purpose of the last commit is to make the default |
Though what would be even better is if all OMERO logs went to stdout/stderr, so just |
|
README.md
Outdated
/config/extra.omero:/opt/omero/server/config/extra.omero:ro | ||
openmicroscopy/omero-server | ||
|
||
Parameters required for initialising the server such as database configuration *must* be set using environment variables. |
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.
initializing
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.
Changed
Re-ran travis to get this green. Merging, untagged. I'd suggest when cc: @mtbc |
Refactor the startup-scripts for OMERO.server:
99-run.sh
script is run, avoiding the requirement to restart the server.