PR#55: Issue #49: Migrate CLI processing to click - modularity/fedmod

modularity / fedmod

#55 Issue #49: Migrate CLI processing to click

Merged 6 years ago by ncoghlan. Opened 6 years ago by ncoghlan.

modularity/ ncoghlan/fedmod issue-49-no-more-argparse into master

Issue #49: Migrate CLI processing to click

Nick Coghlan • 6 years ago

bbb61b6

src/_fedmod/cli.py

file modified

+103 -169

		`@@ -1,184 +1,118 @@`
		`import sys`
		`- import argparse`
		`+ import click`
		`import logging`

		`from .module_generator import ModuleGenerator`
		`from .module_repoquery import ModuleRepoquery`
		`from . import _depchase, _repodata`

		`- # TODO: Switch this over to click (already a dependency for progress bars)`
		`-`
		`- class ModtoolsCLI(object):`
		`- """ Class for processing data from commandline """`
		`-`
		`- @staticmethod`
		`- def build_parser():`
		`- parser = argparse.ArgumentParser(description="Generates module related files")`
		`- base_parser = argparse.ArgumentParser(add_help=False)`
		`- base_parser.add_argument(`
		`- "--verbose",`
		`- "-v",`
		`- action="store_true",`
		`- help="verbose operation"`
		`- )`
		`-`
		`- subparsers = parser.add_subparsers(dest="cmd_name")`
		`-`
		`- parser_rpm2module = subparsers.add_parser(`
		`- 'rpm2module', parents=[base_parser],`
		`- help="Generates modulemd file",`
		`- description="Gets package info and dependencies and creates modulemd file."`
		`- )`
		`- parser_rpm2module.add_argument(`
		`- "--output",`
		`- "-o",`
		`- metavar="FILE",`
		`- dest="output_fname",`
		`- help="Write to FILE instead of stdout."`
		`- )`
		`- parser_rpm2module.add_argument(`
		`- "--build-deps",`
		`- metavar="N",`
		`- default=0,`
		`- dest="build_deps_iterations",`
		`- help="Attempt to ensure N levels of build dependencies (Default: %(default)s)."`
		`- )`
		`- parser_rpm2module.add_argument(`
		`- "pkgs",`
		`- metavar='PKGS',`
		`- nargs='+',`
		`- help="Specify list of packages for module.",`
		`- )`
		`-`
		`- parser_metadata = subparsers.add_parser(`
		`- 'fetch-metadata', parents=[base_parser],`
		`- help="Fetches repository metadata",`
		`- description="Caches needed repository metadata locally"`
		`- )`
		`-`
		`- parser_list_modules = subparsers.add_parser(`
		`- 'list-modules', parents=[base_parser],`
		`- help='Lists all modules.',`
		`- )`
		`-`
		`- parser_list_rpms = subparsers.add_parser(`
		`- 'list-rpms', parents=[base_parser],`
		`- help='Lists all modularized rpm packages.',`
		`- )`
		`- parser_list_rpms.add_argument(`
		`- "--duplicate-only",`
		`- action='store_true',`
		`- help="Only packages that are in more than one modules.",`
		`- )`
		`- parser_list_rpms.add_argument(`
		`- "--list-modules",`
		`- action='store_true',`
		`- help="List modules for every package.",`
		`- )`
		`-`
		`- parser_resolve_deps = subparsers.add_parser(`
		`- 'resolve-deps', parents=[base_parser],`
		`- help='List dependencies of given rpm packages.',`
		`- )`
		`- parser_resolve_deps.add_argument(`
		`- "--module-dependency",`
		`- "-m",`
		`- action="append",`
		`- metavar="MODULE",`
		`- help="Module to be used as a dependency. Can be used multiple times.",`
		`- )`
		`- parser_resolve_deps.add_argument(`
		`- "pkgs",`
		`- metavar='PKGS',`
		`- nargs='+',`
		`- help="Specify list of packages.",`
		`- )`
		`-`
		`- parser_module_packages = subparsers.add_parser(`
		`- 'module-packages', parents=[base_parser],`
		`- help='Lists packages in a given module',`
		`- )`
		`- parser_module_packages.add_argument(`
		`- "module",`
		`- metavar='MODULE',`
		`- help="Name of the module.",`
		`- )`
		`- parser_module_packages.add_argument(`
		`- "--full-nevra",`
		`- action='store_true',`
		`- help="Print the full NEVRA instead of only name.",`
		`- )`
		`-`
		`- parser_where_is_package = subparsers.add_parser(`
		`- 'where-is-package', parents=[base_parser],`
		`- help='Check if a package has been modularized and where',`
		`- )`
		`- parser_where_is_package.add_argument(`
		`- "pkg",`
		`- metavar='PKG',`
		`- help="Name of the package.",`
		`- )`
		`-`
		`- parser_srpm_of_rpm = subparsers.add_parser(`
		`- 'srpm-of-rpm', parents=[base_parser],`
		`- help='Check if a package has been modularized and where',`
		`- )`
		`- parser_srpm_of_rpm.add_argument(`
		`- "pkg",`
		`- metavar='PKG',`
		`- help="Name of the package.",`
		`- )`
		`-`
		`- return parser`
		`-`
		`- def __init__(self, args=None):`
		`- self.parser = ModtoolsCLI.build_parser()`
		`- self.args = self.parser.parse_args(args)`
		`-`
		`- def __getattr__(self, name):`
		`- try:`
		`- return getattr(self.args, name)`
		`- except AttributeError:`
		`- return object.__getattribute__(self, name)`
		`-`
		`+ # fedmod uses click for argument parsing, but currently does its own`
		`+ # standardised exception handling. This also requires handling click's standard`
		`+ # exception types as described in http://click.pocoo.org/5/exceptions/`

		`def run():`
		`try:`
		`- cli = ModtoolsCLI(sys.argv[1:])`
		`- if cli.args.verbose:`
		`- logging.basicConfig(level=logging.INFO)`
		`- if cli.args.cmd_name == 'rpm2module':`
		`- mg = ModuleGenerator(cli.args.pkgs)`
		`- mg.run(cli.args.output_fname, cli.args.build_deps_iterations)`
		`- elif cli.args.cmd_name == 'fetch-metadata':`
		`- _repodata.download_repo_metadata()`
		`- elif cli.args.cmd_name == 'list-modules':`
		`- rq = ModuleRepoquery()`
		`- rq.list_modules()`
		`- elif cli.args.cmd_name == 'list-rpms':`
		`- rq = ModuleRepoquery()`
		`- rq.list_modularized_pkgs(`
		`- duplicate_only=cli.args.duplicate_only,`
		`- list_modules=cli.args.list_modules,`
		`- )`
		`- elif cli.args.cmd_name == 'resolve-deps':`
		`- rq = ModuleRepoquery()`
		`- rq.list_pkg_deps(cli.args.pkgs, cli.args.module_dependency)`
		`- elif cli.args.cmd_name == 'module-packages':`
		`- rq = ModuleRepoquery()`
		`- rq.list_rpms_in_module(cli.args.module, full_nevra=cli.args.full_nevra)`
		`- elif cli.args.cmd_name == 'where-is-package':`
		`- rq = ModuleRepoquery()`
		`- rq.list_modules_for_rpm(cli.args.pkg)`
		`- elif cli.args.cmd_name == 'srpm-of-rpm':`
		`- rq = ModuleRepoquery()`
		`- rq.get_srpm_for_rpm(cli.args.pkg)`
		`-`
		`- except KeyboardInterrupt:`
		`+ rc = _cli_commands.main(sys.argv[1:], standalone_mode=False)`
		`+ except (KeyboardInterrupt, click.Abort):`
		`print('\nInterrupted by user')`
		`+ rc = 1`
		`+ except click.ClickException as e:`
		`+ e.show()`
		`+ rc = e.exit_code`
		`except _repodata.MissingMetadata as e:`
		`print(e, file=sys.stderr)`
		`- sys.exit(2)`
		`+ rc = 2`
		`except Exception as e:`
		`logging.exception("Unexpected exception")`
		`- sys.exit(1)`
		`+ rc = 3`
		`+ sys.exit(rc)`
		`+`
		`+`
		`+ @click.group()`
		`+ @click.option("--verbose", "-v", is_flag=True, default=False,`
		`+ help="Show additional info messages")`
		`+ def _cli_commands(verbose):`
		`+ """Utilities for working with modular Fedora RPM repositories"""`
		`+ if verbose:`
		`+ logging.basicConfig(level=logging.INFO)`
		`+`
		`+`
		`+ # Fetching metadata`
		`+ @_cli_commands.command('fetch-metadata')`
		`+ def fetch_metadata():`
		`+ """Fetch and cache required module and RPM metadata"""`
		`+ _repodata.download_repo_metadata()`
		`+`
		`+`
		`+ # modulemd generation`
		`+ @_cli_commands.command()`
		`+ @click.option("--output", "-o", metavar="FILE", help="Write to FILE instead of stdout.")`
		`+ @click.option("--build-deps", metavar="N", default=0,`
		`+ help="Attempt to ensure N levels of build dependencies (Default: 0).")`
		`+ @click.argument("pkgs", metavar='PKGS', nargs=-1, required=True)`
		`+ def rpm2module(pkgs, output, build_deps):`
		`+ """Generate a draft modulemd from an RPM (or list of RPMs)"""`
		`+ mg = ModuleGenerator(pkgs)`
		`+ mg.run(output, build_deps)`
		`+`
		`+ # Checking availability of dependencies through module streams`
		`+ @_cli_commands.command('resolve-deps')`
		`+ @click.option("--module-dependency", "-m", multiple=True, metavar="MODULE",`
		`+ help="Module to be used as a dependency. Can be given multiple times.")`
		`+ @click.argument("pkgs", metavar='PKGS', nargs=-1, required=True)`
		`+ def resolve_deps(pkgs, module_dependency):`
		`+ """Report dependencies of the given RPM (or list of RPMs)"""`
		`+ rq = ModuleRepoquery()`
		`+ rq.list_pkg_deps(pkgs, module_dependency)`
		`+`
		`+`
		`+ # Listing modules`
		`+ @_cli_commands.command('list-modules')`
		`+ def list_modules():`
		`+ """List the available modules"""`
		`+ rq = ModuleRepoquery()`
		`+ rq.list_modules()`
		`+`
		`+`
		`+ # Looking for RPM duplication across modules`
		`+ @_cli_commands.command('list-rpms')`
		`+ @click.option("--duplicate-only", is_flag=True, default=False,`
		`+ help="Only list packages that are in more than one module.")`
		`+ @click.option("--list-modules", is_flag=True, help="List modules for every package.")`
		`+ def list_rpms(duplicate_only, list_modules):`
		`+ """List RPMs available through modules"""`
		`+ rq = ModuleRepoquery()`
		`+ rq.list_modularized_pkgs(`
		`+ duplicate_only=duplicate_only,`
		`+ list_modules=list_modules,`
		`+ )`
		`+`
		`+ # Checking module contents`
		+ # (ncoghlan): Perhaps `module-rpms` would be more consistent?
		`+ @_cli_commands.command('module-packages')`
		`+ @click.option("--full-nevra", is_flag=True, default=False,`
		`+ help="Print the full NEVRA instead of only name.")`
		`+ @click.argument("module")`
		`+ def module_rpms(module, full_nevra):`
		`+ """Lists RPMS in a given module"""`
		`+ rq = ModuleRepoquery()`
		`+ rq.list_rpms_in_module(module, full_nevra=full_nevra)`
		`+`
		`+`
		`+ # Checking availability of specific packages`
		+ # (ncoghlan): Perhaps `where-is-rpm` would be more consistent?
		`+ @_cli_commands.command('where-is-package')`
		`+ @click.argument("pkg")`
		`+ def which_module(pkg):`
		`+ """Reports which module (if any) provides the given RPM"""`
		`+ rq = ModuleRepoquery()`
		`+ rq.list_modules_for_rpm(pkg)`
		`+`
		`+`
		`+ # Checking what to build to satisfy particular binary dependencies`
		`+ @_cli_commands.command('srpm-of-rpm')`
		`+ @click.argument("pkg")`
		`+ def which_srpm(pkg):`
		`+ """Reports which SRPM produces the given RPM"""`
		`+ rq = ModuleRepoquery()`
		`+ rq.get_srpm_for_rpm(pkg)`

tests/test_cli_ux.py

file modified

+25 -1

		`@@ -1,4 +1,4 @@`
		`- """End-to-end subprocess based testing for the CLI behaviour"""`
		`+ """Tests for the general behaviour of the CLI"""`

		`import pytest`
		`import os`
		`@@ -31,3 +31,27 @@`
		`err_msg = re.sub(b"'.*'", b"'<filename>'", result.stderr)`
		`assert err_msg == expected_err_msg`

		`+ EXPECTED_SUBCOMMANDS = (`
		`+ 'rpm2module',`
		`+ 'fetch-metadata',`
		`+ 'list-modules',`
		`+ 'list-rpms',`
		`+ 'resolve-deps',`
		`+ 'module-packages',`
		`+ 'where-is-package',`
		`+ 'srpm-of-rpm',`
		`+ )`
		`+`
		`+ class TestHelpMessages(object):`
		`+ # We don't test the details of the help messages, as we trust click`
		`+ # to handle that correctly.`
		`+ #`
		`+ # Instead, these tests just ensure that all expected subcommands exist`
		+ # and that they all accept an explicit `--help` option
		`+`
		`+ def test_default_help(self):`
		`+ assert _run_fedmod(["--help"]).returncode == 0`
		`+`
		`+ def test_subcommand_help(self):`
		`+ for subcommand in EXPECTED_SUBCOMMANDS:`
		`+ assert _run_fedmod([subcommand, "--help"]).returncode == 0`

tests/test_module_generator.py

file modified

+30 -70

		`@@ -1,39 +1,30 @@`
		`"""In-process tests for the modulemd generator"""`

		`import pytest`
		`- import os.path`
		`- from _fedmod.cli import ModtoolsCLI`
		`+ from click.testing import CliRunner`
		`+`
		`+ import modulemd`
		`+ from _fedmod.cli import _cli_commands`
		`from _fedmod.module_generator import ModuleGenerator`

		`def _generate_modulemd(rpms, build_deps_iterations=0):`
		`- # TODO: Actually test the CLI arg parsing via a subprocess`
		`- cmd_input = list(['rpm2module'])`
		`- cmd_input.extend(rpms)`
		`- cli = ModtoolsCLI(cmd_input)`
		`- mg = ModuleGenerator(cli.pkgs)`
		`- if len(rpms) == 1:`
		`- output_fname = rpms[0] + '.yaml'`
		`- else:`
		`- output_fname = 'modulemd-output.yaml'`
		`- mg.run(output_fname, build_deps_iterations)`
		`- return mg, output_fname`
		`+ cmd = ['rpm2module']`
		`+ if build_deps_iterations:`
		`+ cmd.extend(['--build-deps', str(build_deps_iterations)])`
		`+ cmd.extend(rpms)`
		`+ runner = CliRunner()`
		`+ result = runner.invoke(_cli_commands, cmd)`
		`+ assert result.exit_code == 0`
		`+ modmd = modulemd.ModuleMetadata()`
		`+ modmd.loads(result.output)`
		`+ return modmd`


		`class TestSinglePackageInput(object):`

		`- def setup(self):`
		`- self.input_rpms = input_rpms = ('grep',)`
		`- self.md, self.output_fname = _generate_modulemd(input_rpms)`
		`-`
		`- def teardown(self):`
		`- os.remove(self.output_fname)`
		`-`
		`def test_generated_modulemd_file(self):`
		`- # File exists with expected name`
		`- assert os.path.isfile(self.input_rpms[0] + '.yaml')`
		`-`
		`- # Check details of generated module metadata`
		`- modmd = self.md.mmd`
		`+ input_rpms = ('grep',)`
		`+ modmd = _generate_modulemd(input_rpms)`

		`# Expected description for 'grep'`
		`assert modmd.summary == "Pattern matching utilities"`
		`@@ -49,32 +40,21 @@`
		`assert modmd.content_licenses == {'GPLv3+'}`

		`# Only given modules are listed in the public API`
		`- assert sorted(modmd.api.rpms) == sorted(self.input_rpms)`
		`+ assert sorted(modmd.api.rpms) == sorted(input_rpms)`

		`# Expected components for grep`
		`- assert set(modmd.components.rpms) == set(self.input_rpms)`
		`+ assert set(modmd.components.rpms) == set(input_rpms)`

		`# Expected module dependencies for grep`
		`assert set(modmd.buildrequires) == set()`
		`assert set(modmd.requires) == {'platform',}`


		`-`
		`class TestMultiplePackageInput(object):`

		`- def setup(self):`
		`- self.input_rpms = input_rpms = ('grep', 'haproxy')`
		`- self.md, self.output_fname = _generate_modulemd(input_rpms)`
		`-`
		`- def teardown(self):`
		`- os.remove(self.output_fname)`
		`-`
		`def test_generated_modulemd_file(self):`
		`- # File exists with expected name`
		`- assert os.path.isfile('modulemd-output.yaml')`
		`-`
		`- # Check details of generated module metadata`
		`- modmd = self.md.mmd`
		`+ input_rpms = ('grep', 'haproxy')`
		`+ modmd = _generate_modulemd(input_rpms)`

		`# Can't generate descriptive metadata when given multiple RPMs`
		`assert modmd.summary == ""`
		`@@ -85,10 +65,10 @@`
		`assert len(modmd.content_licenses) == 0 # This doesn't seem right...`

		`# Only given modules are listed in the public API`
		`- assert sorted(modmd.api.rpms) == sorted(self.input_rpms)`
		`+ assert sorted(modmd.api.rpms) == sorted(input_rpms)`

		`# Expected components for grep + haproxy`
		`- expected_components = set(self.input_rpms)`
		`+ expected_components = set(input_rpms)`
		`assert set(modmd.components.rpms) == expected_components`

		`# Expected module dependencies for grep + haproxy`
		`@@ -98,19 +78,9 @@`

		`class TestRecursiveBuildDeps(object):`

		`- def setup(self):`
		`- self.input_rpms = input_rpms = ('mariadb',)`
		`- self.md, self.output_fname = _generate_modulemd(input_rpms, 100)`
		`-`
		`- def teardown(self):`
		`- os.remove(self.output_fname)`
		`-`
		`def test_generated_modulemd_file(self):`
		`- # File exists with expected name`
		`- assert os.path.isfile('mariadb.yaml')`
		`-`
		`- # Check details of generated module metadata`
		`- modmd = self.md.mmd`
		`+ input_rpms = ('mariadb',)`
		`+ modmd = _generate_modulemd(input_rpms, 100)`

		`# Descriptive metadata`
		`assert modmd.summary == "A community developed branch of MySQL"`
		`@@ -127,14 +97,14 @@`
		`assert modmd.content_licenses == {'GPLv2 with exceptions and LGPLv2 and BSD'}`

		`# Only given modules are listed in the public API`
		`- assert sorted(modmd.api.rpms) == sorted(self.input_rpms)`
		`+ assert sorted(modmd.api.rpms) == sorted(input_rpms)`

		`# MariaDB's complicated test suite poses some real challenges for`
		`# build dependency resolution, even when the generator is kind of`
		`# cheating and relying on the actual Fedora MariaDB module at runtime`

		`# Expected components`
		`- expected_components = set(self.input_rpms)`
		`+ expected_components = set(input_rpms)`
		`expected_components \|= {`
		`'systemtap',`
		`'multilib-rpm-config',`
		`@@ -172,19 +142,9 @@`

		`class TestNonRecursiveBuildDeps(object):`

		`- def setup(self):`
		`- self.input_rpms = input_rpms = ('graphite-web',)`
		`- self.md, self.output_fname = _generate_modulemd(input_rpms)`
		`-`
		`- def teardown(self):`
		`- os.remove(self.output_fname)`
		`-`
		`def test_generated_modulemd_file(self):`
		`- # File exists with expected name`
		`- assert os.path.isfile('graphite-web.yaml')`
		`-`
		`- # Check details of generated module metadata`
		`- modmd = self.md.mmd`
		`+ input_rpms = ('graphite-web',)`
		`+ modmd = _generate_modulemd(input_rpms)`

		`# Descriptive metadata`
		`assert modmd.summary == "A Django web application for enterprise scalable realtime graphing"`
		`@@ -205,10 +165,10 @@`
		`assert modmd.content_licenses == {'ASL 2.0'}`

		`# Only given modules are listed in the public API`
		`- assert sorted(modmd.api.rpms) == sorted(self.input_rpms)`
		`+ assert sorted(modmd.api.rpms) == sorted(input_rpms)`

		`# Expected components`
		`- expected_components = set(self.input_rpms)`
		`+ expected_components = set(input_rpms)`
		`expected_components \|= {`
		`'python-twisted',`
		`'python-simplejson',`

tests/test_module_repoquery.py

file modified

-1

		`@@ -2,7 +2,6 @@`

		`import pytest`
		`import os.path`
		`- from _fedmod.cli import ModtoolsCLI`
		`from _fedmod.module_repoquery import ModuleRepoquery`

		`class TestListingModules(object):`

ncoghlan commented 6 years ago

The main maintainability benefit this gives is access
to click's native subcommand support, which allows
the options and arguments for subcommands to be defined
directly alongside the implementations of those subcommands.

It also provides access to click's native testing infrastructure