Skip to content
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

config xml in etc grid #14

Merged
merged 5 commits into from Dec 2, 2019
Merged

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Aug 30, 2019

This enforces setting OMERODIR so we don't end up with config etc in a different place for omero-py vv omero-web.

To test, pip install this branch... and omero-web from ome/omero-web#85

unset OMERODIR      # Make sure this is NOT set

python3 -m venv py3_venv
source py3_venv/bin/activate

pip install omero-py      # this branch
pip install omero-web   # (include https://github.com/ome/omero-web/pull/85)

omero config set omero.web.debug True
# should fail

@will-moore
Copy link
Member Author

Travis failing:

E                   TypeError: string indices must be integers, not str

target/omero/scripts.py:577: TypeError
----------------------------- Captured stderr call -----------------------------
DEBUG:omero.util.TempFileManager:Added file /root/omero/tmp/omero_root/79/omero9QKtf1.tmp
------------------------------ Captured log call -------------------------------
DEBUG    omero.util.TempFileManager:temp_files.py:259 Added file /root/omero/tmp/omero_root/79/omero9QKtf1.tmp
=========================== short test summary info ============================
FAILED test/unit/scriptstest/test_parse.py::TestParse::testGroupingWithMainExtraDot
FAILED test/unit/scriptstest/test_parse.py::TestParse::testGroupingWithMain
======== 2 failed, 784 passed, 134 skipped, 1 xfailed in 44.11 seconds =========
ERROR: InvocationError for command /src/.tox/py27/bin/pytest -n8 -m 'not broken' -rf test/unit/clitest/ test/unit/fstest/ test/unit/gatewaytest/ test/unit/scriptstest/ (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   py27: commands failed
The command '/bin/sh -c /v/bin/tox' returned a non-zero code: 1
travis_time:end:09f4de60:start=1567176161024349687,finish=1567176292488334700,duration=131463985013
�[0K�[31;1mThe command "docker build -t test ." exited with 1.�[0m

@manics
Copy link
Member

manics commented Sep 2, 2019

Seems OK for now.

In future if we move to a new read-only config system (only the administrator can modify the config, omero can't modify itself) we might need a second variable to separate /etc and /var.

@joshmoore
Copy link
Member

Agreed, and/or functionality to prefix those directories with /omero to get /etc/omero/... etc.

Two questions:

  • Is this more part of fixing up the decoupling and so is part of 5.5.1; or beyond?
  • What would be the impact of not falling back to get_omero_userdir()?

@manics
Copy link
Member

manics commented Sep 3, 2019

Is this more part of fixing up the decoupling and so is part of 5.5.1; or beyond?

Depends on our instructions. If they say you must set OMERODIR to use OMERO.web or OMERO.server then this PR isn't needed, I'm not aware of any reason to support omero config in OMERO.py without OMERO.web or OMERO.server.

Edit: You could also make the case that omero config should be disabled in the absence of web or server.

@joshmoore
Copy link
Member

joshmoore commented Sep 3, 2019

I'm going to exclude for a round of testing on py3-ci. merge-ci passed because it is currently using the versions on pypi. I suspect that this PR is leading to OMERODIR being required such that this takes us into phase 2. If so, a phase 1 version will need to be split out which just creates the directory without changing the lookup logic.

@joshmoore
Copy link
Member

Confirmed. Without this PR, a devspace (and therefore a user) that has not yet set OMERODIR can start a server.

@joshmoore joshmoore added this to In progress in OMERO.server 5.6.0 (Python 3) via automation Sep 5, 2019
@joshmoore
Copy link
Member

@will-moore : could you look at getting this to work under phase 1?

@will-moore
Copy link
Member Author

will-moore commented Sep 26, 2019

This is not required for phase 1 with decoupling when we use omero-web as part of the server build (as before). E.g. (without $OMERDIR set...)

  • At Current a6977678df (HEAD, origin/develop, origin/HEAD) Merge pull request #6102 from joshmoore/fix-scripts
./build.py clean build
cd dist/
bin/omero config append omero.web.server_list '["demo.openmicroscopy.org", 4064, "demo"]'
bin/omero config set omero.web.debug True
bin/omero config set omero.web.application_server development
bin/omero admin start
bin/omero web start

When we run decoupled omero-py and omero-web, this PR isn't required to start the web server (with no settings), but it is required if we want omero-web to use any settings created with omero config set in omero-py, and this requires $OMERODIR because omero-web will look there for settings.

If we want this to work without $OMERODIR then we need to teach omero-web to look in userdir / omero / config.xml instead?

@will-moore
Copy link
Member Author

@joshmoore Could you remove the exclude flag above? Thanks.

@jburel
Copy link
Member

jburel commented Nov 25, 2019

@will-moore done

@will-moore
Copy link
Member Author

This caused the build to fail today since OMERODIR wasn't set.
As discussed, we should never try to guess a location for OMERODIR and we should fail if it's not set.

@will-moore will-moore changed the title Create OMERODIR / etc / grid for config.xml if not exists config xml in etc grid Nov 29, 2019
@manics
Copy link
Member

manics commented Dec 2, 2019

$ omero config get
OMERODIR env variable not set

I think it would be clearer to indicate this is an error e.g. prefix with Error: , otherwise it looks like an info message.

Looks like other plugins that should check for OMERODIR still attempt to continue, e.g.:

$ omero admin diagnostics
FATAL: OMERO directory does not exist: /VIRTUALENV/lib/etc/grid
$ omero web start

OMERO.web starts, but it's using /VIRTUALENV/lib/var

Does each plugin (admin, web) need to be updated separately or can it be done in one place?

@joshmoore
Copy link
Member

Edit: You could also make the case that omero config should be disabled in the absence of web or server.

@manics : can you explain what you meant here? I missed it last time around.

Does each plugin (admin, web) need to be updated separately or can it be done in one place?

I imagine the logic here can become a decorator that gets applied to the multiple plugins (much like #77)

I'd propose getting @manics' Error: / FATAL suggestion merged with this PR as an immediate warning to everyone, and then we can work to refactor it across more plugins.

@manics
Copy link
Member

manics commented Dec 2, 2019

can you explain what you meant here? I missed it last time around.

It's the equivalent of requiring OMERODIR to be set for any command that uses the omero config.

Co-Authored-By: Josh Moore <j.a.moore@dundee.ac.uk>
@joshmoore joshmoore added this to the 5.6.0 milestone Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants