cli: error code discovery

[joshmoore]

Suggested by Damir, a few additional methods on the BaseControl
class permit discovering all errors codes for available plugins.

Example output for the state of this commit:

    $ bin/omero errors
    123	     admin	NOT_WINDOWS	'Not Windows'
    200	     admin	     SETUP	'Error during service user set up:  (%s) %s'
    201	     admin	   RUNNING	'%s is already running. Use stop first'
    201	    server	 NO_CONFIG	'No --Ice.Config provided'
    202	     admin	NO_SERVICE	'%s service deleted.'
    300	     admin	BAD_CONFIG	'Bad configuration: No IceGrid.Node.Data property'
    400	     admin	WIN_CONFIG	'%s is not in this directory. Aborting...      ...'
    666	     admin	  NO_WIN32	'Could not import win32service and/or win32evtlogutil'

see: https://www.openmicroscopy.org/community/viewtopic.php?f=6&t=8676&p=20525#p20525

• agreement on the output format
• API finalization
• review external plugins
• review included plugins

[joshmoore]

Addition of hql.py shows some room for refactoring:

   52	       hql	 BAD_QUERY	'Bad query: %s'
   52	    search	 BAD_QUERY	'Bad query: %s'
   53	       hql	 NOT_ADMIN	'SecurityViolation: Current user is not an admin an...'
   53	    search	 NOT_ADMIN	'SecurityViolation: Current user is not an admin an...'
   67	       hql	  NO_QUIET	'Can't ask for query with --quiet option'
   67	    search	  NO_QUIET	'Can't ask for query with --quiet option'

[sbesson]

Looking at it, overall 👍 about the beginning of the rationalization of these codes. A couple of immediate feedback from pure code review while considering the extension of decoupled CLI plugins to implement this functionality

• at the moment, implementing this API would force a hard-dependency on an OMERO.py 5.5. Or would we want to handle this more gracefully at the plugin level e.g by wrapping self.raise_error( in a try/catch block to remain backwards-compatible?
• for each plugin, the set of errors is serialized as a list of tuples (code, name, message). Could these be declared as a static variable Control.ERRORS? https://github.com/openmicroscopy/openmicroscopy/blob/5a4b1bb6d32dd8588f3be4aee19e7e52977e2a2e/components/tools/OmeroPy/src/omero/cli.py#L1504-1505 could be extended to boostrap the error code registration?

[joshmoore]

by wrapping self.raise_error( in a try/catch block to remain backwards-compatible?

I thought about that, but that leads to a good deal of verbosity.

Could these be declared as a static variable Control.ERRORS?

It'd be a different approach.

[jburel]

at the moment, implementing this API would force a hard-dependency on an OMERO.py 5.5.
This could be reviewed as part of the python decoupling

[jburel]

I think the report format gives a good indication of what is going on

[joshmoore]

Could these be declared as a static variable Control.ERRORS?
It'd be a different approach.

@sbesson : any thoughts? Guess I'm trying to avoid pinning us down to the implementation for the moment. We agree to not change:

     def add_error(self, name, rcode, msg):
     def get_errors(self):
     def raise_error(self, name, *args):
     class Error(object):

in a breaking manner, introducing Error2 and friends if need be.

[sbesson]

I went through the code once again. Overall the new API should keep existing code backwards-compatible i.e. plugins (core or decoupled) should be able to ignore the new API and use self.ctx.die() as previously. When implementing the new API, I think add_error and raise_error are the two main ones that need to be implemented.

At this stage, I am tempted to just get this in and start testing with decoupled plugins e.g. omero-cli-render once OMERO 5.5 is out. Worst case, it turns out to be inefficient and we can deprecate the new Errors class as well as the new methods.

There is also a submodule update as part of the PR, is that on purpose?

[joshmoore]

There is also a submodule update as part of the PR, is that on purpose?

Definitely not. Force-pushed away. Otherwise, 👍

[jburel]

Worst case, it turns out to be inefficient and we can deprecate the new Errors class as well as the new methods.

In that case, do we mark it as "experimental"?

[joshmoore]

In that case, do we mark it as "experimental"?

I defer to you guys. I'm happy to support this, but unless we mark it as "Experimental" we should all be more or less happy with it, since propagating changes to the plugins shouldn't be taken lightly.

[paulkorir]

Do you have an exit code 44 anywhere? I get it when I run omero sessions login <param> && omero delete Image:<id> over ssh (and ssh has no exit 44).

[joshmoore]

No, I'm not seeing an error code 44 either.

[paulkorir]

You might want to consider adding 177 to mean No results found.

(omero) [centos@centos7 ~]$ omero search  Image '^emd_5625-*'
Created session for [email protected]:4064. Idle timeout: 10 min. Current group: system
No results found.
(omero) [centos@centos7 ~]$ echo $?
177

[joshmoore]

@paulkorir : 👍 Will get to ASAP, but as always PRs welcome!

[joshmoore]

Hi @paulkorir, I've started ome/omero-py#266 but it looks like 177 is not registered anywhere. We'll need to look into who or what is raising it to make that PR useful to you. ~J

[joshmoore]

Found the issue for 177. It's the code returned modulo 256:

python -c "import sys; sys.exit(433)"; echo $?
177

See https://stackoverflow.com/questions/4448339/how-to-bypass-the-0-255-range-limit-for-sys-exit-in-python for some more information. I'll likely continue investigating this via ome/omero-py#267