New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arg parsing refactor #50069

Merged
merged 5 commits into from Jan 4, 2019

Conversation

Projects
None yet
6 participants
@abadger
Copy link
Member

abadger commented Dec 18, 2018

SUMMARY

Move parsed arguments into a global Singleton so that they can be referenced from anywhere.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

cli/*
New context object

ADDITIONAL INFORMATION

This is a prerequisite for the become plugins. I'll be rebasing the become plugins Pr on top of this once it's done.

Code at the initial push has adhoc working with the new code but all the other command line tools will be broken. Wanting to upload so other people can see it.

@abadger abadger force-pushed the abadger:arg-parsing-refactor branch from 1920838 to 364cab4 Dec 18, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 18, 2018

The test ansible-test sanity --test pylint [explain] failed with 40 errors:

test/units/cli/test_console.py:30:14: abstract-class-instantiated Abstract class 'ConsoleCLI' with abstract methods instantiated
test/units/cli/test_console.py:35:14: abstract-class-instantiated Abstract class 'ConsoleCLI' with abstract methods instantiated
test/units/cli/test_console.py:45:14: abstract-class-instantiated Abstract class 'ConsoleCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:49:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:102:21: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:106:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:112:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:122:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:135:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:143:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:153:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:163:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:197:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:202:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:207:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:213:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:222:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:228:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:235:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:244:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:250:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:257:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:263:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:271:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_galaxy.py:301:13: abstract-class-instantiated Abstract class 'GalaxyCLI' with abstract methods instantiated
test/units/cli/test_vault.py:43:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:53:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:59:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:68:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:78:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:86:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:96:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:108:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:120:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:131:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:143:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:151:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:159:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:167:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated
test/units/cli/test_vault.py:175:14: abstract-class-instantiated Abstract class 'VaultCLI' with abstract methods instantiated

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: Can't instantiate abstract class DocCLI with abstract methods init_parser, post_process_args

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: Can't instantiate abstract class DocCLI with abstract methods init_parser, post_process_args

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: Can't instantiate abstract class DocCLI with abstract methods init_parser, post_process_args

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: Can't instantiate abstract class DocCLI with abstract methods init_parser, post_process_args

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: Can't instantiate abstract class DocCLI with abstract methods init_parser, post_process_args

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
cat _themes/srtd/static/css/theme.css | sed -e 's/^[ 	]*//g; s/[ 	]*$//g; s/\([:{;,]\) /\1/g; s/ {/{/g; s/\/\*.*\*\///g; /^$/d' | sed -e :a -e '$!N; s/\n\(.\)/\1/; ta' > _themes/srtd/static/css/theme.min.css
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
Makefile:83: recipe for target 'cli' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/generate_man.py", line 253, in <module>
    allvars[cli_name] = opts_docs(cli_class_name, cli_name)
  File "../bin/generate_man.py", line 105, in opts_docs
    cli = cli_klass([])
TypeError: Can't instantiate abstract class ConfigCLI with abstract methods init_parser, post_process_args
make: *** [cli] Error 1

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

test/units/test_arguments.py:30:23: E124 closing bracket does not match visual indentation
test/units/test_arguments.py:127:25: E127 continuation line over-indented for visual indent
test/units/test_arguments.py:128:25: E127 continuation line over-indented for visual indent

click here for bot help

@jborean93 jborean93 removed the needs_triage label Dec 18, 2018

@abadger abadger force-pushed the abadger:arg-parsing-refactor branch from 364cab4 to 51dc2c1 Dec 19, 2018

@abadger abadger force-pushed the abadger:arg-parsing-refactor branch 2 times, most recently from fe4b34d to ac1c56b Dec 19, 2018

@abadger abadger changed the title [WIP] Arg parsing refactor Arg parsing refactor Dec 19, 2018

@abadger

This comment has been minimized.

Copy link
Member

abadger commented Dec 19, 2018

Okay, the major work is done. I have a checklist of items I want to make sure got changed correctly for each cli/* and I don't know if it will pass CI yet but this should now look the way it is supposed to. Once those two things are done, this will be ready for in-depth review from others and I will be able to rebase the become plugins PR on top of this.

@ansibot ansibot added core_review and removed WIP labels Dec 19, 2018

@abadger abadger force-pushed the abadger:arg-parsing-refactor branch 4 times, most recently from ecc1aa7 to 3dbe73b Dec 19, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 19, 2018

The test ansible-test sanity --test pylint [explain] failed with 13 errors:

lib/ansible/cli/console.py:130:11: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:131:24: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:186:24: undefined-variable Undefined variable 'module_name'
lib/ansible/cli/console.py:412:15: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:415:27: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:419:27: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:420:22: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:421:27: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:422:29: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:423:26: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:424:20: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:425:21: undefined-variable Undefined variable 'context'
lib/ansible/cli/console.py:438:51: undefined-variable Undefined variable 'context'

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Dec 19, 2018

@abadger abadger force-pushed the abadger:arg-parsing-refactor branch 5 times, most recently from 4cd824e to 1eb2c19 Dec 19, 2018

@@ -0,0 +1,377 @@
# Copyright: (c) 2018, Ansible Project

This comment has been minimized.

@sivel

sivel Dec 20, 2018

Member

I wonder if this file, and possibly the dir live in the cli dir. It's still heavily tied to the cli.

This comment has been minimized.

@abadger

abadger Dec 20, 2018

Member

Yeah. I thought about that with just the file at first but the current bin/ansible code made me decide against it. I think the directory will work because I think the bin/ansible code is limited to files inside of the cli/ dir. If that's so I'll move it.

This comment has been minimized.

@abadger

abadger Dec 20, 2018

Member

Moved in latest version

@@ -1,4 +1,4 @@
# Copyright: (c) 2017, Ansible Project
# Copyright: (c) 2017-2018, Ansible Project

This comment has been minimized.

@sivel

sivel Dec 20, 2018

Member

I'd leave the hyphenating off. I believe we have a doc somewhere that indicates to just use the start year. Otherwise this becomes hard to manage, and we may forget to advance this.

This comment has been minimized.

@abadger

abadger Dec 20, 2018

Member

Thanks! I must have skipped that paragraph the last time I read the licensing advice doc.

This comment has been minimized.

@abadger

abadger Dec 20, 2018

Member

Fixed to only have 2017.

CLIARGS = CLIArgs({})


class _Context:

This comment has been minimized.

@sivel

sivel Dec 20, 2018

Member

What are the plans for this class? Upon further inspection, I don't see it being used.

This comment has been minimized.

@abadger

abadger Dec 20, 2018

Member

tldr; Yeah, it could be removed.

More info:

@nitzmahone's criticisms of the initial idea of global singleton's was that it prevents us from having different arguments in different threads/workers down the road. Having a context would fix that (Basically reinstating the old way of passing in the cli args from the toplevel into the functions which do the work but being expandable to more than just cli args). When I was struggling with the unittests I started implementing a Context object so that I could pass it in for unittests to workaround the single-immutable aspect of the CLIARGS. After adopting your idea to mess with the Singleton's private data instead, though, I didn't need to continue down the Context path.

The Context object did point out to me the need for separate GlobalCLIArgs and CliArgs objects which I've now implemented but we don't need to keep the Context code as proof of that. I should probably add more information to the CliArgs class to justify its existence if I remove Context from here, though.

This comment has been minimized.

@abadger

abadger Dec 20, 2018

Member

Removed in latest version

@ansibot ansibot added core_review and removed needs_revision labels Dec 20, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 20, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "/root/ansible/bin/ansible-doc", line 32, in <module>
    from ansible import context
  File "/root/ansible/lib/ansible/context.py", line 18, in <module>
    from ansible.cli.arguments.context_objects import CLIArgs, GlobalCLIArgs
  File "/root/ansible/lib/ansible/cli/__init__.py", line 20, in <module>
    from ansible import context
ImportError: cannot import name context

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with the error:

Command "ansible-doc -t cache jsonfile memcached memory mongodb pickle redis yaml" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "/root/ansible/bin/ansible-doc", line 32, in <module>
    from ansible import context
  File "/root/ansible/lib/ansible/context.py", line 18, in <module>
    from ansible.cli.arguments.context_objects import CLIArgs, GlobalCLIArgs
  File "/root/ansible/lib/ansible/cli/__init__.py", line 20, in <module>
    from ansible import context
ImportError: cannot import name context

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
cat _themes/srtd/static/css/theme.css | sed -e 's/^[ 	]*//g; s/[ 	]*$//g; s/\([:{;,]\) /\1/g; s/ {/{/g; s/\/\*.*\*\///g; /^$/d' | sed -e :a -e '$!N; s/\n\(.\)/\1/; ta' > _themes/srtd/static/css/theme.min.css
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
PYTHONPATH=../../lib ../bin/dump_keywords.py --template-dir=../templates --output-dir=rst/reference_appendices/ -d ./keyword_desc.yml
Makefile:87: recipe for target 'keywords' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/dump_keywords.py", line 12, in <module>
    from ansible.playbook import Play
  File "/root/ansible/lib/ansible/playbook/__init__.py", line 27, in <module>
    from ansible.playbook.play import Play
  File "/root/ansible/lib/ansible/playbook/play.py", line 26, in <module>
    from ansible.playbook.base import Base
  File "/root/ansible/lib/ansible/playbook/base.py", line 23, in <module>
    from ansible.utils.vars import combine_vars, isidentifier, get_unique_id
  File "/root/ansible/lib/ansible/utils/vars.py", line 30, in <module>
    from ansible import context
  File "/root/ansible/lib/ansible/context.py", line 18, in <module>
    from ansible.cli.arguments.context_objects import CLIArgs, GlobalCLIArgs
  File "/root/ansible/lib/ansible/cli/__init__.py", line 23, in <module>
    from ansible.inventory.manager import InventoryManager
  File "/root/ansible/lib/ansible/inventory/manager.py", line 32, in <module>
    from ansible.inventory.data import InventoryData
  File "/root/ansible/lib/ansible/inventory/data.py", line 27, in <module>
    from ansible.inventory.host import Host
  File "/root/ansible/lib/ansible/inventory/host.py", line 23, in <module>
    from ansible.utils.vars import combine_vars, get_unique_id
ImportError: cannot import name 'combine_vars'
make: *** [keywords] Error 1

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/alicloud/ali_instance.py:0:0: E304 Unknown DOCUMENTATION error, see TRACE: cannot import name 'combine_vars'

click here for bot help

bin/ansible Outdated
have_cli_options = cli is not None and cli.options is not None
have_cli_options = False
if context.CLIARGS:
have_cli_options = True

This comment has been minimized.

@sivel

sivel Jan 3, 2019

Member

Just an FYI, this could be consolidated to something like have_cli_options = bool(context.CLIARGS)

This comment has been minimized.

@abadger

abadger Jan 3, 2019

Member

Done in latest commit

@sivel

This comment has been minimized.

Copy link
Member

sivel commented Jan 3, 2019

Just as an FYI, the slack callback plugin and the CallbackBase will break with this change. Only the slack callback plugin is utilizing from __main__ import cli and then cli.options.

Some third party modules may be using it as well, so this would be a backwards incompatible change, so we might need a porting guide entry too.

I can fix the slack callback and CallbackBase after this is merged.

@abadger

This comment has been minimized.

Copy link
Member

abadger commented Jan 3, 2019

Callback plugins fixed in: 6f933e4

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 3, 2019

@ansibot ansibot added docs docsite and removed stale_ci labels Jan 3, 2019

abadger added some commits Dec 18, 2018

Save the command line arguments into a global context
* Once cli args are parsed, they're constant.  So, save the parsed args
  into the global context for everyone else to use them from now on.
* Port cli scripts to use the CLIARGS in the context
* Refactor call to parse cli args into the run() method
* Fix unittests for changes to the internals of CLI arg parsing
* Port callback plugins to use context.CLIARGS
  * Got rid of the private self._options attribute
  * Use context.CLIARGS in the individual callback plugins instead.
  * Also output positional arguments in default and unixy plugins
  * Code has been simplified since we're now dealing with a dict rather
    than Optparse.Value
Split up the base_parser function
The goal of breaking apart the base_parser() function is to get rid of
a bunch of conditionals and parameters in the code and, instead, make
code look like simple composition.

When splitting, a choice had to be made as to whether this would operate
by side effect (modifying a passed in parser) or side effect-free
(returning a new parser everytime).

Making a version that's side-effect-free appears to be fighting with the
optparse API (it wants to work by creating a parser object, configuring
the object, and then parsing the arguments with it) so instead, make it
clear that our helper functions are modifying the passed in parser by
(1) not returning the parser and (2) changing the function names to be
more clear that it is operating by side-effect.

Also move all of the generic optparse code, along with the argument
context classes, into a new subdirectory.
Cleanups and fixes to cli
* Mark methods which are really functions as staticmethod
* Fix calls to other staticmethods to use the subclass rather than the
  base class so that any inheritance overriding will be honored.
* Remove unnecessary logic and dead code
* Fix a typo in a docstring of how to implement subclass init_parser()
  methods
* Call superclass's post_process_args in ansible-doc
* Fix copyright comment according to suggested practice
Move the arguments module into cli/ and context_objects into utils
* Note: Python2 is not as intelligent at detecting false import loops as
  Python3.  context_objects.py cannot be added to cli/arguments because it
  would set up an import loop between cli/__init__.py,
  cli/arguments/context_objects.py, and context.py on Python2.

ci_complete

@abadger abadger force-pushed the abadger:arg-parsing-refactor branch from e2366d3 to 0d989af Jan 3, 2019

@abadger abadger merged commit 27c7d5b into ansible:devel Jan 4, 2019

1 check passed

Shippable Run 100583 status is SUCCESS.
Details

@abadger abadger deleted the abadger:arg-parsing-refactor branch Jan 4, 2019

@abadger

This comment has been minimized.

Copy link
Member

abadger commented Jan 4, 2019

Merged to devel for the 2.8 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment