argparse: add positional=false option #33164

liewegas · 2020-02-09T02:24:47Z

Allow argdesc to contain 'positional=false', which makes an arg not parsed positionally, requiring the --foo[= [value syntax.

Fixes: https://tracker.ceph.com/issues/42113

Replaces #31042
Based on top of #33135

liewegas · 2020-02-09T02:26:18Z

@matthewoliver what do you think?

liewegas · 2020-02-09T02:27:49Z

Note that I think we can fix https://tracker.ceph.com/issues/42113 in a better way: allow --format to be specified for any command. It's silly that we have to specify it as a param for that one 'orchestrator device ls'.

matthewoliver · 2020-02-09T06:45:28Z

Kk, will flip it around :) makes sense. Matt

…

On Sun., 9 Feb. 2020, 1:27 pm Sage Weil ***@***.*** wrote: Note that I think we can fix https://tracker.ceph.com/issues/42113 in a better way: allow --format to be specified for any command. It's silly that we have to specify it as a param for that one 'orchestrator device ls'. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33164?email_source=notifications&email_token=AAAK3S4GNVJIXQD5WCJBZ2TRB5S2NA5CNFSM4KR6RWKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELGAWIQ#issuecomment-583797538>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAK3S4IH7CAWYJMCOOPL7DRB5S2NANCNFSM4KR6RWKA> .

src/common/cmdparse.cc

tchaikov · 2020-02-10T03:26:40Z

the comment posted at #33135 also applies.

liewegas · 2020-02-10T13:47:09Z

hrm, ceph.in chokes on unexpected keys in the description:

Traceback (most recent call last):
  File "bin/ceph", line 1271, in <module>
    retval = main()
  File "bin/ceph", line 1050, in main
    return do_extended_help(parser, childargs, target, ' '.join(childargs))
  File "bin/ceph", line 416, in do_extended_help
    return help_for_target(target, partial)
  File "bin/ceph", line 413, in help_for_target
    return help_for_sigs(outbuf.decode('utf-8'), partial)
  File "bin/ceph", line 391, in help_for_sigs
    sys.stdout.write(format_help(parse_json_funcsigs(sigs, 'cli'),
  File "/home/sage/src/ceph3/src/pybind/ceph_argparse.py", line 847, in parse_json_funcsigs
    cmd['sig'] = parse_funcsig(cmd['sig'])
  File "/home/sage/src/ceph3/src/pybind/ceph_argparse.py", line 797, in parse_funcsig
    **kwargs))
  File "/home/sage/src/ceph3/src/pybind/ceph_argparse.py", line 673, in __init__
    self.instance = self.t(**self.typeargs)
TypeError: __init__() got an unexpected keyword argument 'positional'

src/common/cmdparse.cc

matthewoliver · 2020-02-11T03:53:40Z

src/pybind/ceph_argparse.py

@@ -1024,6 +1085,7 @@ def validate(args, signature, flags=0, partial=False):
                    validate_one(kwarg_v, kwarg_desc)
                    matchcnt += 1
                    store_arg(kwarg_desc, d)
+                    matchcnt += 1


Incrementing matchcnt is now redundant, back when the patch was first created it was required. But since I've pushed a PR that has since merged that did just this to fix #33004 (now just needs to be back ported)

tchaikov · 2020-02-18T09:12:24Z

src/pybind/ceph_argparse.py

@@ -442,7 +442,7 @@ class CephOsdName(CephArgtype):

    osd.<id>, or <id>, or *, where id is a base10 int
    """
-    def __init__(self):
+    def __init__(self, **kwargs):


i don't understand this change, as argdesc.__init__() is the only place where we construct these CephArgtype instances. but since this method's signature looks like

def __init__(self, t, name=None, n=1, req=True, positional=True, **kwargs):

any unknown parameter should be consumed by CephArgtype already. actually after reverting this change and applying following patch

diff --git a/src/pybind/ceph_argparse.py b/src/pybind/ceph_argparse.py index b8d4664cf8..8782b6a70a 100644 --- a/src/pybind/ceph_argparse.py +++ b/src/pybind/ceph_argparse.py @@ -509,7 +509,7 @@ class CephBool(CephArgtype): A boolean argument, values may be case insensitive 'true', 'false', '0', '1'. In keyword form, value may be left off (implies true). """ - def __init__(self, strings='', **kwargs): + def __init__(self, strings=''): self.strings = strings.split('|') def valid(self, s, partial=False):

i ran following tests:

$ bin/ceph config set global mon_warn_on_pool_pg_num_not_power_of_two false --force=false $ bin/ceph config set global mon_warn_on_pool_pg_num_not_power_of_two true $ bin/ceph orch device ls gnit json Error ENOENT: No orchestrator configured (try `ceph orch set backend`)

sebastian-philipp · 2020-02-27T10:05:57Z

needs rebase

sebastian-philipp · 2020-04-02T14:27:10Z

ping?

liewegas · 2020-04-02T16:17:31Z

Rebased, but htis depends on #34272

smithfarm · 2020-04-13T14:38:01Z

Just wondering, does this PR have any relation to https://tracker.ceph.com/issues/43114 ?

liewegas · 2020-04-13T15:10:41Z

Just wondering, does this PR have any relation to https://tracker.ceph.com/issues/43114 ?

Not related. I think this is a useful cleanup/step forward for the CLI overall, but I was nervous about merging just prior to octopus. Now would be a good time to rebase and retest this, but hopefully someone can adopt it?

liewegas · 2020-04-13T15:10:55Z

retest this please

This patch adds an new parameter to the argdesc class, `param` that when set to `true` will force the `--name=` syntax in the generated helpstr. The `param=true` is also added to the `fomat` option of the `ceph orchestrator device ls` command so the specified usage wont result in a cli syntax error. This patch also modifies the ceph client to remove any param=true argdesc's from the srgs and deals with them non positionally. Fixes: https://tracker.ceph.com/issues/42113 Signed-off-by: Matthew Oliver <moliver@suse.com>

Signed-off-by: Sage Weil <sage@redhat.com>

If the argdesc we are passed by the server has fields we do not recognize, we currently choke with something like Traceback (most recent call last): File "bin/ceph", line 1271, in <module> retval = main() File "bin/ceph", line 1050, in main return do_extended_help(parser, childargs, target, ' '.join(childargs)) File "bin/ceph", line 416, in do_extended_help return help_for_target(target, partial) File "bin/ceph", line 413, in help_for_target return help_for_sigs(outbuf.decode('utf-8'), partial) File "bin/ceph", line 391, in help_for_sigs sys.stdout.write(format_help(parse_json_funcsigs(sigs, 'cli'), File "/home/sage/src/ceph3/src/pybind/ceph_argparse.py", line 847, in parse_json_funcsigs cmd['sig'] = parse_funcsig(cmd['sig']) File "/home/sage/src/ceph3/src/pybind/ceph_argparse.py", line 797, in parse_funcsig **kwargs)) File "/home/sage/src/ceph3/src/pybind/ceph_argparse.py", line 673, in __init__ self.instance = self.t(**self.typeargs) TypeError: __init__() got an unexpected keyword argument 'positional' At least, most types do: CephBool does not. Make all of the types tolerate extra (new) fields. This will make future additions less painful. Signed-off-by: Sage Weil <sage@redhat.com>

There are lots of CephBool parameters this applies too, but they already look like non-positional arguments (as far as I can tell, at least). There is *one* other command: COMMAND("pg dump_stuck " "name=stuckops,type=CephChoices,strings=inactive|unclean|stale|undersized|degraded,n=N,req=false " \ "name=threshold,type=CephInt,req=false", In this case previously you could do 'pg dump_stuck inactive 123' and the parser could tell 123 was not part of the stuckops list because it wasn't in the enum. With this PR you now have to do ceph pg dump_stuck inactive --threshold 123 IMO this is an improvement as it more closely aligns with how other parsers work and is less ambiguous. Signed-off-by: Sage Weil <sage@redhat.com>

Signed-off-by: Sage Weil <sage@redhat.com>

stale · 2020-06-17T03:52:44Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

stale · 2020-09-20T04:26:10Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

liewegas requested review from sebastian-philipp, dmick and jdurgin February 9, 2020 02:25

liewegas added common core labels Feb 9, 2020

liewegas requested a review from tchaikov February 9, 2020 02:26

tchaikov reviewed Feb 10, 2020

View reviewed changes

src/common/cmdparse.cc Outdated Show resolved Hide resolved

liewegas added the DNM label Feb 10, 2020

liewegas force-pushed the wip-cli branch from 0066a76 to a8a70aa Compare February 10, 2020 22:54

tchaikov reviewed Feb 11, 2020

View reviewed changes

src/common/cmdparse.cc Outdated Show resolved Hide resolved

liewegas force-pushed the wip-cli branch from a8a70aa to e84d48c Compare February 11, 2020 02:59

matthewoliver reviewed Feb 11, 2020

View reviewed changes

liewegas force-pushed the wip-cli branch from e84d48c to 745a1b4 Compare February 11, 2020 13:13

liewegas added wip-sage2-testing and removed DNM labels Feb 13, 2020

liewegas requested a review from tchaikov February 14, 2020 15:59

shyukri mentioned this pull request Feb 15, 2020

nautilus: ceph_argparse: increment matchcnt on kwargs #33160

Merged

liewegas removed the wip-sage2-testing label Feb 18, 2020

tchaikov reviewed Feb 18, 2020

View reviewed changes

sebastian-philipp added the needs-rebase label Feb 27, 2020

liewegas force-pushed the wip-cli branch from d023c1d to 6121822 Compare April 2, 2020 16:17

Matthew Oliver and others added 7 commits April 17, 2020 17:45

pybind/ceph_argparse: only add - variant if _ is present

31efdf0

Signed-off-by: Sage Weil <sage@redhat.com>

pybind/ceph_argparse: param=true -> positional=false

b8e864a

Signed-off-by: Sage Weil <sage@redhat.com>

common/cmdparse: do not break pre-pacific clients with 'positional'

b020e2a

Signed-off-by: Sage Weil <sage@redhat.com>

pybind/ceph_argparse: make CephBool.valid tolerate a bool

64e21c2

Signed-off-by: Sage Weil <sage@redhat.com>

liewegas force-pushed the wip-cli branch from 6121822 to 64e21c2 Compare April 17, 2020 22:45

stale bot added the stale label Jun 17, 2020

stale bot closed this Sep 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

argparse: add positional=false option #33164

argparse: add positional=false option #33164

liewegas commented Feb 9, 2020 •

edited

liewegas commented Feb 9, 2020

liewegas commented Feb 9, 2020

matthewoliver commented Feb 9, 2020 via email

tchaikov commented Feb 10, 2020

liewegas commented Feb 10, 2020

matthewoliver Feb 11, 2020

tchaikov Feb 18, 2020

sebastian-philipp commented Feb 27, 2020

sebastian-philipp commented Apr 2, 2020

liewegas commented Apr 2, 2020

smithfarm commented Apr 13, 2020

liewegas commented Apr 13, 2020

liewegas commented Apr 13, 2020

stale bot commented Jun 17, 2020

stale bot commented Sep 20, 2020

argparse: add positional=false option #33164

argparse: add positional=false option #33164

Conversation

liewegas commented Feb 9, 2020 • edited

liewegas commented Feb 9, 2020

liewegas commented Feb 9, 2020

matthewoliver commented Feb 9, 2020 via email

tchaikov commented Feb 10, 2020

liewegas commented Feb 10, 2020

matthewoliver Feb 11, 2020

Choose a reason for hiding this comment

tchaikov Feb 18, 2020

Choose a reason for hiding this comment

sebastian-philipp commented Feb 27, 2020

sebastian-philipp commented Apr 2, 2020

liewegas commented Apr 2, 2020

smithfarm commented Apr 13, 2020

liewegas commented Apr 13, 2020

liewegas commented Apr 13, 2020

stale bot commented Jun 17, 2020

stale bot commented Sep 20, 2020

liewegas commented Feb 9, 2020 •

edited