Arvados

Check other CLI tools for the same bug while we're at it.

17840-unparsed-args @ 40f551004ab4e5f1d8ab02ddb55dca225ee8f6ac -- developer-run-tests: #2779

retrying workbench1 tests at developer-run-tests-apps-workbench-integration: #2952

Tom Clegg wrote:

17840-unparsed-args @ 40f551004ab4e5f1d8ab02ddb55dca225ee8f6ac -- developer-run-tests: #2779

retrying workbench1 tests at developer-run-tests-apps-workbench-integration: #2952

OK, there seems to be a fair bit going on here. Looking at keep-balance, before the patch:

 ./keep-balance -x
flag provided but not defined: -x
Usage of ./keep-balance:
  -commit-confirmed-fields
        update collection fields (replicas_confirmed, storage_classes_confirmed, etc.) (default true)
  -commit-pulls
        send pull requests (make more replicas of blocks that are underreplicated or are not in optimal rendezvous probe order)
  -commit-trash
        send trash requests (delete unreferenced old blocks, and excess replicas of overreplicated blocks)
  -config file
        Site configuration file (default may be overridden by setting an ARVADOS_CONFIG environment variable) (default "/etc/arvados/config.yml")
  -dump
        dump details for each block to stdout
  -legacy-crunch-dispatch-slurm-config file
        Legacy crunch-dispatch-slurm configuration file (default "/etc/arvados/crunch-dispatch-slurm/crunch-dispatch-slurm.yml")
  -legacy-git-httpd-config file
        Legacy arv-git-httpd configuration file (default "/etc/arvados/git-httpd/git-httpd.yml")
  -legacy-keepbalance-config file
        Legacy keep-balance configuration file (default "/etc/arvados/keep-balance/keep-balance.yml")
  -legacy-keepproxy-config file
        Legacy keepproxy configuration file (default "/etc/arvados/keepproxy/keepproxy.yml")
  -legacy-keepstore-config file
        Legacy keepstore configuration file (default "/etc/arvados/keepstore/keepstore.yml")
  -legacy-keepweb-config file
        Legacy keep-web configuration file (default "/etc/arvados/keep-web/keep-web.yml")
  -legacy-ws-config file
        Legacy arvados-ws configuration file (default "/etc/arvados/ws/ws.yml")
  -once
        balance once and then exit
  -pprof [addr]:port
        serve Go profile data at [addr]:port
  -skip-legacy
        Don't load legacy config files
  -version
        Write version information to stdout and exit 0

With this patch:

 ./keep-balance -x
flag provided but not defined: -x
Usage of ./keep-balance:
  -commit-confirmed-fields
        update collection fields (replicas_confirmed, storage_classes_confirmed, etc.) (default true)
  -commit-pulls
        send pull requests (make more replicas of blocks that are underreplicated or are not in optimal rendezvous probe order)
  -commit-trash
        send trash requests (delete unreferenced old blocks, and excess replicas of overreplicated blocks)
  -config file
        Site configuration file (default may be overridden by setting an ARVADOS_CONFIG environment variable) (default "/etc/arvados/config.yml")
  -dump
        dump details for each block to stdout
  -legacy-crunch-dispatch-slurm-config file
        Legacy crunch-dispatch-slurm configuration file (default "/etc/arvados/crunch-dispatch-slurm/crunch-dispatch-slurm.yml")
  -legacy-git-httpd-config file
        Legacy arv-git-httpd configuration file (default "/etc/arvados/git-httpd/git-httpd.yml")
  -legacy-keepbalance-config file
        Legacy keep-balance configuration file (default "/etc/arvados/keep-balance/keep-balance.yml")
  -legacy-keepproxy-config file
        Legacy keepproxy configuration file (default "/etc/arvados/keepproxy/keepproxy.yml")
  -legacy-keepstore-config file
        Legacy keepstore configuration file (default "/etc/arvados/keepstore/keepstore.yml")
  -legacy-keepweb-config file
        Legacy keep-web configuration file (default "/etc/arvados/keep-web/keep-web.yml")
  -legacy-ws-config file
        Legacy arvados-ws configuration file (default "/etc/arvados/ws/ws.yml")
  -once
        balance once and then exit
  -pprof [addr]:port
        serve Go profile data at [addr]:port
  -skip-legacy
        Don't load legacy config files
  -version
        Write version information to stdout and exit 0
ERRO[0000] error parsing command line flags: flag provided but not defined: -x 

The error message is now added at the bottom which is nice. Any chance we can easily get rid of the one at the top?

And with an invalid/extra argument, we now get:

./keep-balance x
ERRO[0000] unrecognized command line arguments: [x]     

For consistency's sake, should we also print the help text when this happens?

I'm not sure what to think about the formatting of the errors (the leading ERRO\[0000\] is a bit wonky). Is it easy to omit that? Maybe replace with a more human friendly 'Error: ' ? Maybe with a blank line between the help text and the error message, to make it stand out more visually?

Otherwise, nice cleanup!

developer-run-tests: #2791

• de-duplicate much of the argument-parsing code and unify behavior across various programs
• "-h" and "-help" print the usage notes, and exit 0
• bad flags and missing/extra positional arguments cause an error message on stderr, and exit 2, but do not obscure the error message by printing the usage notes before/after

Tom Clegg wrote:

17840-unparsed-args @ 47c3faf1e26be21190eeee7f266d44eb33a0aeb6 -- developer-run-tests: #2791

• de-duplicate much of the argument-parsing code and unify behavior across various programs
• "-h" and "-help" print the usage notes, and exit 0
• bad flags and missing/extra positional arguments cause an error message on stderr, and exit 2, but do not obscure the error message by printing the usage notes before/after

This looks great! Any reason `cmd/arvados-package/cmd.go` was not given the same treatment? LGTM otherwise.

developer-run-tests: #2799

• same treatment to arvados-package
• merged main

Tom Clegg wrote:

17840-unparsed-args @ fb96637bf76fe8779e7a7e58f052b8f55ed76f4f -- developer-run-tests: #2799

• same treatment to arvados-package
• merged main

LGTM, thanks!