Use subcommands for lock,release,check

[pgagne]

Part of #19

made use of argparse.subparser to implement subcommands. Also various related changed detailed below.

So locking a resource aws-resource-1 with signoff ptest1 and priortity 3 now looks like this :

rlock  -s <url> -t <token> lock aws-resource-1 ptest1 3

my hope is in the future we can get rid of requiring -s and -t with some sort of config file.

Details

Arguments that are required by certain things (signoff, search string, etc) are now positional args
This makes it easy to determine if they've been specified and error out if not. Eliminating common
user issues. Server args --server-url form, but that should change if we start using a config file or something.

Added defaults for --interval and --attempts based on the ones in ResourceLocker class

Added short form for most common params (-s, -t, etc)

Here is what --help looks like:

$ rlock --help
usage: rlock [-h] -s SERVER_URL -t TOKEN [--resume-on-connection-error] [-i INTERVAL] {release,check,lock} ...

positional arguments:
  {release,check,lock}
    release             Release a resource
    check               check a resource
    lock                Lock a resource

options:
  -h, --help            show this help message and exit
  -s SERVER_URL, --server-url SERVER_URL
                        The URL of the Resource Locker Server
  -t TOKEN, --token TOKEN
                        Token of the user that creates API calls
  --resume-on-connection-error
                        Use this argument in case you don't want to break queue execution in the middle of waiting for queue status being FINISHED
  -i INTERVAL, --interval INTERVAL
                        How many seconds to wait between each call (default 15)

$ rlock lock --help
usage: rlock lock [-h] [-a ATTEMPTS] [-l LINK] SEARCH SIGN_OFF PRIORITY

positional arguments:
  SEARCH                Specify the label or the name of the lockable resource
  SIGN_OFF              locking or releasing a resource requires signoff
  PRIORITY              Specify the level of priority the resource should be locked

options:
  -h, --help            show this help message and exit
  -a ATTEMPTS, --attempts ATTEMPTS
                        How many times to create an API call that will check for a free resource (default 120)
  -l LINK, --link LINK  Specify the link of the CI/CD pipeline that locks the resource

$ rlock check --help
usage: rlock check [-h] SEARCH

positional arguments:
  SEARCH      Specify the label or the name of the lockable resource

options:
  -h, --help  show this help message and exit

$ rlock release --help
usage: rlock release [-h] SIGN_OFF

positional arguments:
  SIGN_OFF    locking or releasing a resource requires signoff

options:
  -h, --help  show this help message and exit

@jimdevops19 let me know what you think

[pgagne]

@LNST-project Please review as well if you can.

[jtluka]

Could you add some additional description of what SIGN_OFF actually means? Is it an arbitrary string that is used in release after locking a resource? The current description that it is required is obvious, since it's mandatory parameter.

[pgagne]

Could you add some additional description of what SIGN_OFF actually means? Is it an arbitrary string that is used in release after locking a resource? The current description that it is required is obvious, since it's mandatory parameter.

Yeah i kinda just copied what was there before. I tryed to come up with one but they where all too long.

@jimdevops19 Do you have a short phrase to describe what the signoff is.

[olichtne]

just providing some comments from a quick look...

[olichtne]

this comment is doubled and both times in the incorrect place, i think you wanted to say that the parser is the parent of all subcommands, but the comment is located at the search_parser and at the signoff_parser instead.

[olichtne]

i think these arguments since they're shared by all of the sub-commands and relate to the generic configuration of the server, they should just be added to the parser instead of sharing them via parents to all the individual sub-parsers.

I think that results in the difference of:

./my_cli --server-opt  ACTION --action-opt

vs.

./my_cli ACTION --server-opt --action-opt

but i'm not sure... i haven't really used this pattern before. Based on the linked stackoverflow questions I think the parents way of sharing options is meant for sub-commands where some share the options and some don't, these options however look like they're always optional for all sub-commands...

though i guess that's personal preference...

[pgagne]

Yeah I went back and forth a few times on this, I think i ran into an issue adding them to parser and decided to switch back to the way it is now. It might be possible I just didn't have a chance to fully debug it and wanted to get something out.

I my opinion though things like --server and --token really should be coming from some sort of config file so you dont have to type them everytime. We still want them to be options but I think it gets kinda unwieldy having them be part of the command line all the time.

[jimdevops19]

I am a bit concerned about having one letter shortcut for possibly two reasons (maybe su could be a better option):

1 ) Maybe we might want to save the -s for shortening the signoff which is already a mandatory argument

2 ) Maybe some other arguments in the future that will be single-worded and start with an s as well

[pgagne]

Would -u for url be better?

Also I think it might be okay to not shorten it, but i do think we should add support for some sort of config file so you dont need to enter it (and the token, etc) all the time. then we could just have it be --server

I was curious does your team commonly deal with multiple resource locker instances in production?