Merge lp:~jameinel/juju-ci-tools/common-main into lp:juju-ci-tools

[John A Meinel]

This is a request-for-comments about some work I started doing.

Specifically, I started to write a new assess_* module and realized that I was copy & pasting a lot of boilerplate code around parsing arguments and setting up the environment and saving logs if there are any problems.

So I wanted to factor this out into a common helper that can just get reused.

I wanted to check if people are in favor of this before I went too far on it.

One big potential concern is that I'm more comfortable adding 'named' arguments rather than positional arguments, but some of them are going to still be required (environment name, log directory, etc).
I can put them as positional arguments, but then we'd want to figure out how to do that in a common fashion. (Is juju_path always the first position on the command line, or is it always the first position after the command's custom args, etc.)
I went with passing in an ArgumentParser rather than just a custom args func because I would then also need to start passing in all the other help text that you want to have unique to the script (the __init__ to ArgumentParser takes a lot of that stuff).

I don't think this is ready to land as is, because nothing is using it. The actual 'standard_main' function isn't directly tested, because it triggers make_client and bootstrap, etc. But I noticed the main() functions of other scripts weren't tested either, and at least this way there would be 1 untested code path, instead of 10 of them.

[Aaron Bentley (abentley)]

Hi John. Been ages since I reviewed your code!

I'm not comfortable with mandatory "optional" arguments like --environment. It's clearer if mandatory arguments are positional. The argparse docs note: Required options are generally considered bad form because users expect options to be optional, and thus they should be avoided when possible.

See also inline comments.

[Aaron Bentley (abentley)]

I believe this is superseded by BootstrapManager.

Unmerged revisions

991. By John A Meinel on 2015-06-18

Start working on a common 'main' implementation.

I was about to copy and paste main and parse_args for yet another assess function.
It seemed useful to have a common implementation so we don't have to touch them all the time.

=== modified file 'jujupy.py'
--- jujupy.py	2015-06-15 19:14:14 +0000
+++ jujupy.py	2015-06-18 09:16:03 +0000
@@ -996,3 +996,62 @@
         self.last_group = group
         self.ticks = 0
         self.wrap_offset = lead_length if lead_length < self.wrap_width else 0
+
+
+def add_standard_args(parser):
+    """Add standard arguments to a python Argument parser"""
+    parser.add_argument(
+        '--debug', action='store_true', default=False,
+        help='Use --debug juju logging.')
+    parser.add_argument(
+        '--juju-path', default='/usr/bin', help='Directory your juju binary lives in.')
+    parser.add_argument(
+        '--environment', '-e', help='Juju environment name to run tests in.')
+    parser.add_argument('--logs', default='./logs', help='Directory to store logs in.')
+    parser.add_argument('--temp-env-name', default=None,
+        help='Temporary environment name to use for this test.')
+
+
+def standard_main(action_func, parser):
+    """A helpful main function that handles setting up an environment and client.
+
+    This handles all the boilerplate setup and teardown and log saving that all
+    scripts should have. You supply the actual actions to be run on the environment,
+    but if there are any errors the logs will be gathered, etc.
+
+    :param action_func: A function that takes a 'client' as a parameter and
+        runs whatever is necessary on the environment.
+    :param parser: An ArgumentParser that has been initialized with whatever
+        custom arguments you need to support. We will also add support for the
+        standard parameters that we need for make_client() and handling saving
+        of log files, etc.
+    """
+    add_standard_args(parser)
+    args = parser.parse_args()
+    log_dir = args.logs
+
+    client = make_client(
+        args.juju_path, args.debug, args.environment, args.temp_env_name)
+    client.destroy_environment()
+    juju_home = get_juju_home()
+    bootstrap_host = None
+    try:
+        with temp_bootstrap_env(juju_home, client):
+            client.bootstrap()
+        bootstrap_host = get_machine_dns_name(client, 0)
+        client.get_status(60)
+        # This is the standard boilerplate. Bootstrap an environment, run
+        # status to make sure that we are up and running.
+        action_func(client)
+    except Exception as e:
+        logging.exception(e)
+        try:
+            if bootstrap_host is None:
+                bootstrap_host = parse_new_state_server_from_error(e)
+            dump_env_logs(client, bootstrap_host, log_dir)
+        except Exception as e:
+            print_now("exception while dumping logs:\n")
+            logging.exception(e)
+        sys.exit(1)
+    finally:
+        client.destroy_environment()

=== modified file 'test_jujupy.py'
--- test_jujupy.py	2015-06-15 19:14:14 +0000
+++ test_jujupy.py	2015-06-18 09:16:03 +0000
@@ -1,5 +1,9 @@
 __metaclass__ = type
 
+from argparse import (
+    ArgumentParser,
+    Namespace,
+)
 from collections import defaultdict
 from contextlib import contextmanager
 from datetime import (
@@ -27,6 +31,7 @@
     NoSuchEnvironment,
 )
 from jujupy import (
+    add_standard_args,
     CannotConnectEnv,
     Environment,
     EnvJujuClient,
@@ -2398,3 +2403,36 @@
     def test_parse_new_state_server_from_error_no_output(self):
         address = parse_new_state_server_from_error(Exception())
         self.assertIs(None, address)
+
+
+class TestAddStandardArgs(TestCase):
+
+    def setUp(self):
+        super(TestAddStandardArgs, self).setUp()
+        self.parser = ArgumentParser('test parser')
+        add_standard_args(self.parser)
+
+    def assertArgs(self, args, **changes):
+        namespace = Namespace(
+            juju_path='/usr/bin', environment=None, logs='./logs',
+            temp_env_name=None, debug=False)
+        for key, value in changes.items():
+            setattr(namespace, key, value)
+        args = self.parser.parse_args(args)
+        self.assertEqual(args, namespace)
+
+    def test_no_args(self):
+        self.assertArgs([]) # No arg changes
+
+    def test_environment(self):
+        self.assertArgs(["-e", "foo"], environment='foo')
+        self.assertArgs(["--environment", "bar"], environment='bar')
+
+    def test_debug(self):
+        self.assertArgs(["--debug"], debug=True)
+
+    def test_juju_path(self):
+        self.assertArgs(["--juju-path", "/home/foo/bar"], juju_path='/home/foo/bar')
+
+    def test_log_path(self):
+        self.assertArgs(["--logs", "/home/foo/logs"], logs='/home/foo/logs')