Positional arguments not handled correctly

[John Perkins]

Something is preventing positional=True from working correctly. When positional=True is set, the option is not set, regardless of where the argument ends up in the command. Please see [0] for more details.

[0] line 60: https://review.openstack.org/#/c/384559/49/oslo_config/validator.py

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/450928

[Dolph Mathews (dolph)]

There's two issues with that one StrOpt that I've identified.

Firstly, marking it as both positional and required results in an argparse argument that is positional and optional.

Secondly, including a hyphen in the option name results in a hyphen in the destination variable name in the argument namespace (unlike other destinations produced by oslo_config.cfg), making oslo_config.cfg unable to later retrieve the value (because it's retrieving the intended destination, with an underscore, rather than the actual destination, with a hyphen).

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/450956

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/451947

[OpenStack Infra (hudson-openstack)]

Change abandoned by Dolph Mathews (<email address hidden>) on branch: master
Review: https://review.openstack.org/450928
Reason: Superceded by https://review.openstack.org/#/c/451947/

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/450956
Committed: https://git.openstack.org/cgit/openstack/oslo.config/commit/?id=b5f76a2ce8323ae89e85b599c2aec5a89a49eabd
Submitter: Zuul
Branch: master

commit b5f76a2ce8323ae89e85b599c2aec5a89a49eabd
Author: Dolph Mathews <email address hidden>
Date: Tue Mar 28 19:48:48 2017 +0000

    Unit tests to illustrate positional argument bug

    This patch does not provide a fix, but instead serves to illustrate
    several use cases where positional arguments do not behave correctly on
    the CLI.

    Change-Id: Ibdb05066b95a285f6618c861eb4d38465dbf0d02
    Related-Bug: 1676989

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/451933
Committed: https://git.openstack.org/cgit/openstack/oslo.config/commit/?id=ec84eeda526adc20dcabdf6e704958cc06773dd3
Submitter: Zuul
Branch: master

commit ec84eeda526adc20dcabdf6e704958cc06773dd3
Author: Dolph Mathews <email address hidden>
Date: Thu Mar 30 17:56:13 2017 +0000

    Refactor unit tests for CLI usage

    Prior to this change, CLI usage was hardcoded into the config opt
    fixture used for all tests. That CLI usage described two required
    positional arguments that were not actually part of the argument parser
    (FOO and BAR), but were instead just made up to have a valid-looking
    usage description.

    The trouble is that you can't then extend that argument parser with
    additional real arguments, and then test the configuration of those
    arguments by inspecting the *real* usage and --help output from
    argparse. For example, a unit test could not assert whether argparse was
    configured correctly by oslo.config, because there's simply no way to
    know when the usage was just statically set to the arbitrary values of
    "FOO BAR."

    This patch moves that specific unit test coverage (overriding usage with
    something arbitrary) into a dedicated unit test, while removing the
    arbitrary usage from all other unit tests. Instead, those unit tests now
    make assertions against the argparser's real configuration, hence
    options like --config-dir and --config-file now appear in those tests,
    because those options are included in every instance of
    cfg.ConfigOpts().

    Change-Id: I54ba989768d074a5f24897299c85bd35fa1cbd1a
    Related-Bug: 1676989

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/451947
Committed: https://git.openstack.org/cgit/openstack/oslo.config/commit/?id=18d1617caa5a7380e2298b1e647ef110c9d574be
Submitter: Zuul
Branch: master

commit 18d1617caa5a7380e2298b1e647ef110c9d574be
Author: Dolph Mathews <email address hidden>
Date: Thu Mar 30 18:24:31 2017 +0000

    Assume positional arguments are required

    The 'positional' keyword specifically applies to oslo.config's argparse
    support. Unlike oslo.config, argparse assumes that all positional
    arguments are required by default, and you have to explicitly tell it
    that a positional argument is optional if you'd like to opt into that
    behavior.

    This patch adopts that same behavior for oslo.config. When you define an
    option to be non-positional (oslo.config's default, designed for config
    files), then oslo.config makes that option optional:

    However, when you define an option to be positional, oslo.config assumes
    that the option is primarily going to be used on the CLI and thus sets
    it as required, by default.

    This change in behavior has the side effect of allowing argparse to
    enforce required arguments on the CLI *while* parsing arguments, instead
    of depending on oslo.config to detect the condition *after* argparse has
    been allowed to parse "invalid" arguments. argparse correctly raises a
    SystemExit in this case, and prints the actual command usage and a "hey,
    you forgot this required argument", instead of allowing oslo.config to
    dump a backtrace to the CLI with a context-less error message
    ("context-less" in that no useful CLI usage information is dumped along
    with the crash to help you correct the condition).

    Change-Id: Ifdc6918444fe72f7e1649483c237cce64b4c72d8
    Partial-Bug: 1676989