Volans has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/382481 )
Change subject: CLI: extract parser definition from parse_args()
......................................................................
CLI: extract parser definition from parse_args()
* Extract the ArgumentParser definition from parse_args() into a
get_parser() function for easier testability and documentation
generation.
* Uniform help messages in ArgumentParser options.
Change-Id: I99a65ba374860e479c7a1046f4aa72a898ab8730
---
M cumin/cli.py
M cumin/tests/unit/test_cli.py
2 files changed, 94 insertions(+), 42 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/operations/software/cumin
refs/changes/81/382481/1
diff --git a/cumin/cli.py b/cumin/cli.py
index 86209b5..ded2d2a 100644
--- a/cumin/cli.py
+++ b/cumin/cli.py
@@ -55,11 +55,12 @@
"""Custom KeyboardInterrupt exception class for the SIGINT signal
handler."""
-def parse_args(argv=None):
- """Parse command line arguments and return them.
+def get_parser():
+ """Create and return the command line arguments parser.
- Arguments:
- argv: the list of arguments to use. If None, the command line ones are
used [optional, default: None]
+ Returns:
+ argparse.ArgumentParser: the parser object.
+
"""
sync_mode = 'sync'
async_mode = 'async'
@@ -69,13 +70,14 @@
transports_names = [name for _, name, ispkg in
pkgutil.iter_modules(transports.__path__) if not ispkg]
parser = argparse.ArgumentParser(
+ prog='cumin',
description='Cumin CLI - Automation and orchestration framework
written in Python',
epilog='More details at https://wikitech.wikimedia.org/wiki/Cumin')
parser.add_argument('-c', '--config', default='/etc/cumin/config.yaml',
help='configuration file. [default:
/etc/cumin/config.yaml]')
- parser.add_argument('--global-timeout', type=int, default=None,
+ parser.add_argument('--global-timeout', type=int,
help='Global timeout in seconds (int) for the whole
execution. [default: None (unlimited)]')
- parser.add_argument('-t', '--timeout', type=int, default=None,
+ parser.add_argument('-t', '--timeout', type=int,
help=('Timeout in seconds (int) for the the execution
of every command in each host. '
'[default: None (unlimited)]'))
parser.add_argument('-m', '--mode', choices=(sync_mode, async_mode),
@@ -84,47 +86,65 @@
'-p/--success-percentage is reached. In async
mode, execute on each host independently '
'from each other, the list of commands, aborting
the execution on any given host at the '
'first command that fails.'))
- parser.add_argument('-p', '--success-percentage', type=int,
choices=xrange(101), metavar='0-100', default=100,
+ parser.add_argument('-p', '--success-percentage', type=int,
choices=xrange(101), metavar='PCT', default=100,
help=(('Percentage threshold to consider an execution
unit successful. Required in sync mode, '
- 'optional in async mode when -b/--batch-size is
used. [default: 100]')))
+ 'optional in async mode when -b/--batch-size is
used. Accepted values are integers '
+ 'in the range 0-100. [default: 100]')))
parser.add_argument('-b', '--batch-size', type=int,
help=('The commands will be executed with a sliding
batch of this size. The batch mode depends '
'on the -m/--mode option when multiple commands
are specified. In sync mode the first '
'command is executed in batch to all hosts
before proceeding with the next one. In async '
'mode all commands are executed on the first
batch of hosts, proceeding with the next '
'hosts as soon as one host completes all the
commands. The -p/--success-percentage is '
- 'checked before starting the execution in each
hosts.'))
- parser.add_argument('-s', '--batch-sleep', type=float, default=None,
+ 'checked before starting the execution in each
host. [default: None (# of hosts)]'))
+ parser.add_argument('-s', '--batch-sleep', type=float,
help=('Sleep in seconds (float) to wait before
starting the execution on the next host when '
'-b/--batch-size is used. [default: None]'))
parser.add_argument('-x', '--ignore-exit-codes', action='store_true',
- help='USE WITH CAUTION! Treat any executed command as
successful, ignoring the exit codes.')
- parser.add_argument('-o', '--output', choices=OUTPUT_FORMATS,
help='Specify a different output format.')
- parser.add_argument('-i', '--interactive', action='store_true', help='Drop
into a Python shell with the results.')
+ help=('USE WITH CAUTION! Treat any executed command as
successful, ignoring the exit codes. '
+ '[default: False]'))
+ parser.add_argument('-o', '--output', choices=OUTPUT_FORMATS,
+ help='Specify a different output format. [default:
None]')
+ parser.add_argument('-i', '--interactive', action='store_true',
+ help='Drop into a Python shell with the results.
[default: False]')
parser.add_argument('--force', action='store_true',
- help='USE WITH CAUTION! Force the execution without
confirmation of the affected hosts. ')
+ help=('USE WITH CAUTION! Force the execution without
confirmation of the affected hosts. '
+ '[default: False]'))
parser.add_argument('--backend', choices=backends_names,
help=('Override the default backend selected in the
configuration file for this execution. The '
'backend-specific configuration must be already
present in the configuration file. '
- '[optional]'))
+ '[default: None]'))
parser.add_argument('--transport', choices=transports_names,
help=('Override the default transport selected in the
configuration file for this execution. '
'The transport-specific configuration must
already be present in the configuration file. '
- '[optional]'))
+ '[default: None]'))
parser.add_argument('--dry-run', action='store_true',
- help='Do not execute any command, just return the list
of matching hosts and exit.')
+ help=('Do not execute any command, just return the
list of matching hosts and exit. '
+ '[default: False]'))
parser.add_argument('--version', action='version', version='%(prog)s
{version}'.format(version=cumin.__version__))
- parser.add_argument('-d', '--debug', action='store_true', help='Set log
level to DEBUG.')
+ parser.add_argument('-d', '--debug', action='store_true', help='Set log
level to DEBUG. [default: False]')
parser.add_argument('--trace', action='store_true',
- help='Set log level to TRACE, a custom logging level
intended for development debugging.')
+ help=('Set log level to TRACE, a custom logging level
intended for development debugging. '
+ '[default: False]'))
parser.add_argument('hosts', metavar='HOSTS_QUERY', help='Hosts selection
query')
parser.add_argument('commands', metavar='COMMAND', nargs='*',
help='Command to be executed. If no commands are
speficied, --dry-run is set.')
- if argv is None:
- parsed_args = parser.parse_args()
- else:
- parsed_args = parser.parse_args(argv)
+ return parser
+
+
+def parse_args(argv):
+ """Parse command line arguments, validate and return them.
+
+ Arguments:
+ argv: the list of command line arguments to use.
+
+ Returns:
+ argparse.Namespace: the parsed arguments.
+
+ """
+ parser = get_parser()
+ parsed_args = parser.parse_args(argv)
# Validation and default values
num_commands = len(parsed_args.commands)
@@ -334,7 +354,7 @@
# Define a help function h() that will be available in the interactive
shell to print the help message.
# The name is to not shadow the Python built-in help() that might be
usefult too to inspect objects.
def h(): # pylint: disable=unused-variable,invalid-name
- """Helper function for the interactive shell."""
+ """Print the help message in interactive shell."""
tqdm.write(INTERACTIVE_BANNER)
code.interact(banner=INTERACTIVE_BANNER, local=locals())
elif args.output is not None:
@@ -344,12 +364,31 @@
return exit_code
+def validate_config(config):
+ """Perform validation of mandatory keys in the configuration.
+
+ Arguments:
+ config (dict): the loaded configuration to validate.
+
+ Raises:
+ cumin.CuminError: if a mandatory configuration key is missing.
+
+ """
+ if 'log_file' not in config:
+ raise cumin.CuminError("Missing required parameter 'log_file' in the
configuration file '{config}'".format(
+ config=config))
+
+
def main(argv=None):
"""CLI entry point. Execute commands on hosts according to arguments.
Arguments:
- argv: the list of arguments to use. If None, the command line ones are
used [optional, default: None]
+ argv: the list of command line arguments to use. If not specified it
will be automatically taken from sys.argv
+ [optional, default: None]
"""
+ if argv is None:
+ argv = sys.argv[1:]
+
signal.signal(signal.SIGINT, sigint_handler)
colorama.init()
@@ -358,11 +397,7 @@
args = parse_args(argv)
user = get_running_user()
config = cumin.Config(args.config)
-
- if 'log_file' not in config:
- raise cumin.CuminError(("Missing required parameter 'log_file' in
the configuration file "
- "'{config}'").format(config=args.config))
-
+ validate_config(args.config)
setup_logging(config['log_file'], debug=args.debug, trace=args.trace)
except cumin.CuminError as e:
stderr(e)
diff --git a/cumin/tests/unit/test_cli.py b/cumin/tests/unit/test_cli.py
index 9979fb4..957f53e 100644
--- a/cumin/tests/unit/test_cli.py
+++ b/cumin/tests/unit/test_cli.py
@@ -1,10 +1,13 @@
"""CLI tests."""
+import argparse
+
from logging import DEBUG, INFO
import mock
import pytest
from cumin import cli, CuminError, LOGGING_TRACE_LEVEL_NUMBER, transports
+
# Environment variables
_ENV = {'USER': 'root', 'SUDO_USER': 'user'}
@@ -23,19 +26,22 @@
assert args.commands == ['command1', 'command2']
+def test_get_parser():
+ """Calling get_parser() should return a populated argparse.ArgumentParser
object."""
+ parser = cli.get_parser()
+ assert isinstance(parser, argparse.ArgumentParser)
+ assert parser.prog == 'cumin'
+
+
def test_parse_args_ok():
"""A standard set of command line parameters should be properly parsed
into their respective variables."""
- args = cli.parse_args(argv=_ARGV)
+ args = cli.parse_args(_ARGV)
_validate_parsed_args(args)
-
- with mock.patch.object(cli.sys, 'argv', ['progname'] + _ARGV):
- args = cli.parse_args()
- _validate_parsed_args(args)
def test_parse_args_no_commands():
"""If no commands are specified, dry-run mode should be implied."""
- args = cli.parse_args(argv=_ARGV[:-2])
+ args = cli.parse_args(_ARGV[:-2])
_validate_parsed_args(args, no_commands=True)
@@ -43,7 +49,7 @@
"""If mode is not speficied with multiple commands, parsing the args
should raise a parser error."""
index = _ARGV.index('-m')
with pytest.raises(SystemExit):
- cli.parse_args(argv=_ARGV[:index] + _ARGV[index + 1:])
+ cli.parse_args(_ARGV[:index] + _ARGV[index + 1:])
def test_get_running_user():
@@ -133,7 +139,7 @@
@mock.patch('cumin.cli.sys.stdout.isatty')
def test_get_hosts_ok(isatty, mocked_raw_input, stderr):
"""Calling get_hosts() should query the backend and return the list of
hosts."""
- args = cli.parse_args(argv=['D{host1}', 'command1'])
+ args = cli.parse_args(['D{host1}', 'command1'])
config = {'backend': 'direct'}
isatty.return_value = True
@@ -159,7 +165,7 @@
@mock.patch('cumin.cli.sys.stdout.isatty')
def test_get_hosts_no_tty_ko(isatty, stderr):
"""Calling get_hosts() without a TTY should raise CuminError if --dry-run
or --force are not specified."""
- args = cli.parse_args(argv=['D{host1}', 'command1'])
+ args = cli.parse_args(['D{host1}', 'command1'])
config = {'backend': 'direct'}
isatty.return_value = False
with pytest.raises(CuminError, match='Not in a TTY but neither DRY-RUN nor
FORCE mode were specified'):
@@ -171,7 +177,7 @@
@mock.patch('cumin.cli.sys.stdout.isatty')
def test_get_hosts_no_tty_dry_run(isatty, stderr):
"""Calling get_hosts() with or without a TTY with --dry-run should return
an empty list."""
- args = cli.parse_args(argv=['--dry-run', 'D{host1}', 'command1'])
+ args = cli.parse_args(['--dry-run', 'D{host1}', 'command1'])
config = {'backend': 'direct'}
assert cli.get_hosts(args, config) == []
isatty.return_value = True
@@ -183,7 +189,7 @@
@mock.patch('cumin.cli.sys.stdout.isatty')
def test_get_hosts_no_tty_force(isatty, stderr):
"""Calling get_hosts() with or without a TTY with --force should return
the list of hosts."""
- args = cli.parse_args(argv=['--force', 'D{host1}', 'command1'])
+ args = cli.parse_args(['--force', 'D{host1}', 'command1'])
config = {'backend': 'direct'}
assert cli.get_hosts(args, config) == cli.NodeSet('host1')
isatty.return_value = True
@@ -195,10 +201,21 @@
@mock.patch('cumin.cli.stderr')
def test_run(stderr, transport):
"""Calling run() should query the hosts and execute the commands on the
transport."""
- args = cli.parse_args(argv=['--force', 'D{host1}', 'command1'])
+ args = cli.parse_args(['--force', 'D{host1}', 'command1'])
config = {'backend': 'direct', 'transport': 'clustershell'}
cli.run(args, config)
assert transport.new.call_args[0][0] is config
assert isinstance(transport.new.call_args[0][1], transports.Target)
assert transport.new.call_args[1]['logger'] is cli.logger
assert stderr.called
+
+
+def test_validate_config_valid():
+ """A valid config should be validated without raising exception."""
+ cli.validate_config({'log_file': '/var/log/cumin/cumin.log'})
+
+
+def test_validate_config_invalid():
+ """An invalid config should raise CuminError."""
+ with pytest.raises(CuminError, match='Missing required parameter'):
+ cli.validate_config({})
--
To view, visit https://gerrit.wikimedia.org/r/382481
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I99a65ba374860e479c7a1046f4aa72a898ab8730
Gerrit-PatchSet: 1
Gerrit-Project: operations/software/cumin
Gerrit-Branch: master
Gerrit-Owner: Volans <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
