Merge lp:~nskaggs/juju-ci-tools/add-snap-to-default-juju-args into lp:juju-ci-tools

[Nicholas Skaggs]

Tweak things to allow running with no arguments if you have the snap version of juju installed. Extra tests were added to clarify the order; GOPATH/bin/juju > /usr/bin/juju > /snap/bin/juju.

[lp:~nskaggs/juju-ci-tools/add-snap-to-default-juju-args]

1876. By Nicholas Skaggs on 2017-02-08

simplify

1877. By Nicholas Skaggs on 2017-02-08

Verbose mode enabled for function comment

[Aaron Bentley (abentley)]

I don't think this is right. If there is a default juju, it should be the juju returned by "which juju", i.e. the one in your PATH. If you have the snap installed, the snap juju should be in your PATH.

client_from_config already gives you this value if juju_path is None.

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

Funny you bring up 'which juju', as indeed I wanted to modify this to use the same logic. The hardcoded paths feel silly when it's really just $PATH/$GOPATH + juju.

I don't remember why I stepped back from that approach, other than it must not have worked. It was the original implementation. hmmm.

[Nicholas Skaggs (nskaggs)]

I attempted to reuse get_full_path for this, it can be nice, make it a utility function. However, it caused spiraling headaches all across the testsuite; into every single test. TestParseArgs everywhere needed a new mock to prevent the popen call. Given that, I think I'd prefer this.

[Aaron Bentley (abentley)]

You don't need to do more, you need to do less. This change produces only 5 test failures.
=== modified file 'utility.py'
--- utility.py 2017-01-19 04:47:42 +0000
+++ utility.py 2017-02-13 20:19:51 +0000
@@ -338,7 +338,7 @@
                         help='Full path to the Juju binary. By default, this'
                         ' will use $GOPATH/bin/juju or /usr/bin/juju in that'
                         ' order.',
- default=_generate_default_binary())
+ default=None)

 def add_basic_testing_arguments(parser, using_jes=False, deadline=True,

[Nicholas Skaggs (nskaggs)]

I consider the sane defaults useful and essential for letting other folks easily run tests. I think this represents the least invasive option for adding support for snapped juju, but I'm open to other ideas not yet explored. I don't think removing the ability to not require arguments is the right direction.

review: Needs Information

[Aaron Bentley (abentley)]

Nothing in what I said would remove sane defaults. By setting default to None, our tests will default to whatever juju is in the $PATH, be that a snap or whatever is installed.

[lp:~nskaggs/juju-ci-tools/add-snap-to-default-juju-args]

1878. By Nicholas Skaggs on 2017-02-13

swap get_full_path to get_juju_path in utility

1879. By Nicholas Skaggs on 2017-03-01

revert 1878

1880. By Nicholas Skaggs on 2017-03-01

wip

1881. By Nicholas Skaggs on 2017-03-01

wip

1882. By Nicholas Skaggs on 2017-03-01

fixed logging, fixed juju binary path

1883. By Nicholas Skaggs on 2017-03-01

lint

1884. By Nicholas Skaggs on 2017-03-01

merge trunk

[Aaron Bentley (abentley)]

Thanks.

I'm a little unclear why you moved _generate_default_clean_dir, but I'm fine with it.

I do think that the leading underscore is incorrect. This is a public function meant to be invoked outside utility.py, not a private function. But that's a quibble.

review: Approve

[lp:~nskaggs/juju-ci-tools/add-snap-to-default-juju-args]

1885. By Nicholas Skaggs on 2017-03-01

make _generate_default_clean_dir public

=== modified file 'assess_public_clouds.py'
--- assess_public_clouds.py	2017-02-22 16:08:27 +0000
+++ assess_public_clouds.py	2017-03-01 20:36:24 +0000
@@ -22,8 +22,9 @@
     )
 from utility import (
     _clean_dir,
+    generate_default_clean_dir,
+    add_arg_juju_bin,
     configure_logging,
-    _generate_default_binary,
     LoggedException,
     _to_deadline,
     )
@@ -101,10 +102,7 @@
     """Parse all arguments."""
     parser = ArgumentParser(
         description='Assess basic quality of public clouds.')
-    parser.add_argument('juju_bin', nargs='?',
-                        help='Full path to the Juju binary. By default, this'
-                        ' will use $GOPATH/bin/juju or /usr/bin/juju in that'
-                        ' order.', default=_generate_default_binary())
+    add_arg_juju_bin(parser)
     parser.add_argument('logs', nargs='?', type=_clean_dir,
                         help='A directory in which to store logs. By default,'
                         ' this will use the current directory', default=None)
@@ -126,8 +124,7 @@
 
 def default_log_dir(settings):
     if settings.logs is None:
-        settings.logs = BootstrapManager._generate_default_clean_dir(
-            'assess_public_clouds')
+        settings.logs = generate_default_clean_dir('assess_public_clouds')
 
 
 def main():
 
=== modified file 'deploy_stack.py'
--- deploy_stack.py	2017-02-28 18:47:38 +0000
+++ deploy_stack.py	2017-03-01 20:36:24 +0000
@@ -9,12 +9,6 @@
 except ImportError:
     from contextlib import ExitStack as nested
 
-
-from datetime import (
-    datetime,
-)
-
-import errno
 import glob
 import logging
 import os
@@ -63,6 +57,7 @@
     make_substrate_manager,
 )
 from utility import (
+    generate_default_clean_dir,
     add_basic_testing_arguments,
     configure_logging,
     ensure_deleted,
@@ -473,6 +468,8 @@
     args = deploy_job_parse_args()
     configure_logging(args.verbose)
     series = args.series
+    if not args.logs:
+        args.logs = generate_default_clean_dir(args.temp_env_name)
     if series is None:
         series = 'precise'
     charm_series = series
@@ -706,29 +703,9 @@
         return self.controller_strategy.tear_down_client
 
     @classmethod
-    def _generate_default_clean_dir(cls, temp_env_name):
-        """Creates a new unique directory for logging and returns name"""
-        logging.info('Environment {}'.format(temp_env_name))
-        test_name = temp_env_name.split('-')[0]
-        timestamp = datetime.now().strftime("%Y%m%d%H%M%S")
-        log_dir = os.path.join('/tmp', test_name, 'logs', timestamp)
-
-        try:
-            os.makedirs(log_dir)
-            logging.info('Created logging directory {}'.format(log_dir))
-        except OSError as e:
-            if e.errno == errno.EEXIST:
-                logging.warn('"Directory {} already exists'.format(log_dir))
-            else:
-                raise('Failed to create logging directory: {} ' +
-                      log_dir +
-                      '. Please specify empty folder or try again')
-        return log_dir
-
-    @classmethod
     def from_args(cls, args):
         if not args.logs:
-            args.logs = cls._generate_default_clean_dir(args.temp_env_name)
+            args.logs = generate_default_clean_dir(args.temp_env_name)
 
         # GZ 2016-08-11: Move this logic into client_from_config maybe?
         if args.juju_bin == 'FAKE':
 
=== modified file 'tests/test_assess_bootstrap.py'
--- tests/test_assess_bootstrap.py	2017-02-28 12:55:09 +0000
+++ tests/test_assess_bootstrap.py	2017-03-01 20:36:24 +0000
@@ -219,7 +219,8 @@
         with assess_bootstrap_cxt():
             with self.sub_assess_mocks() as (base_mock, metadata_mock,
                                              to_mock):
-                assess_bootstrap(args)
+                with patch('jujupy.ModelClient.get_full_path'):
+                    assess_bootstrap(args)
         self.assertEqual(1, base_mock.call_count)
         self.assertEqual(0, metadata_mock.call_count)
         self.assertEqual(0, to_mock.call_count)
@@ -229,7 +230,8 @@
         with assess_bootstrap_cxt():
             with self.sub_assess_mocks() as (base_mock, metadata_mock,
                                              to_mock):
-                assess_bootstrap(args)
+                with patch('jujupy.ModelClient.get_full_path'):
+                    assess_bootstrap(args)
         self.assertEqual(0, base_mock.call_count)
         self.assertEqual(1, metadata_mock.call_count)
         self.assertEqual(0, to_mock.call_count)
@@ -239,7 +241,8 @@
         with assess_bootstrap_cxt():
             with self.sub_assess_mocks() as (base_mock, metadata_mock,
                                              to_mock):
-                assess_bootstrap(args)
+                with patch('jujupy.ModelClient.get_full_path'):
+                    assess_bootstrap(args)
         self.assertEqual(0, base_mock.call_count)
         self.assertEqual(0, metadata_mock.call_count)
         self.assertEqual(1, to_mock.call_count)
 
=== modified file 'tests/test_assess_public_clouds.py'
--- tests/test_assess_public_clouds.py	2017-02-10 20:39:24 +0000
+++ tests/test_assess_public_clouds.py	2017-03-01 20:36:24 +0000
@@ -46,9 +46,10 @@
 class TestParseArgs(TestCase):
 
     def test_parse_args(self):
-        args = parse_args([])
+        with patch('utility.os.getenv', return_value=False):
+            args = parse_args([])
         self.assertEqual(Namespace(
-            deadline=None, debug=False, juju_bin='/usr/bin/juju', logs=None,
+            deadline=None, debug=False, juju_bin=None, logs=None,
             start=0, cloud_regions=None,
             ), args)
 
@@ -117,18 +118,16 @@
 
     def test_default_log_dir(self):
         settings = Namespace(logs=None)
-        with patch(
-                'deploy_stack.BootstrapManager._generate_default_clean_dir',
-                return_value='/tmp12345') as clean_dir_mock:
+        with patch('assess_public_clouds.generate_default_clean_dir',
+                   return_value='/tmp12345') as clean_dir_mock:
             default_log_dir(settings)
         self.assertEqual('/tmp12345', settings.logs)
         clean_dir_mock.assert_called_once_with(_LOCAL)
 
     def test_default_log_dir_provided(self):
         settings = Namespace(logs='/tmpABCDE')
-        with patch(
-                'deploy_stack.BootstrapManager._generate_default_clean_dir',
-                autospec=True) as clean_dir_mock:
+        with patch('assess_public_clouds.generate_default_clean_dir',
+                   autospec=True) as clean_dir_mock:
             default_log_dir(settings)
         self.assertEqual('/tmpABCDE', settings.logs)
         self.assertFalse(clean_dir_mock.called)
 
=== modified file 'tests/test_deploy_stack.py'
--- tests/test_deploy_stack.py	2017-02-28 16:02:17 +0000
+++ tests/test_deploy_stack.py	2017-03-01 20:36:24 +0000
@@ -1149,7 +1149,7 @@
 
     def test_deploy_job_changes_series_with_win(self):
         args = Namespace(
-            series='windows', temp_env_name=None, env=None, upgrade=None,
+            series='windows', temp_env_name='windows', env=None, upgrade=None,
             charm_prefix=None, bootstrap_host=None, machine=None, logs=None,
             debug=None, juju_bin=None, agent_url=None, agent_stream=None,
             keep_env=None, upload_tools=None, with_chaos=None, jes=None,
@@ -1162,7 +1162,7 @@
 
     def test_deploy_job_changes_series_with_centos(self):
         args = Namespace(
-            series='centos', temp_env_name=None, env=None, upgrade=None,
+            series='centos', temp_env_name='centos', env=None, upgrade=None,
             charm_prefix=None, bootstrap_host=None, machine=None, logs=None,
             debug=None, juju_bin=None, agent_url=None, agent_stream=None,
             keep_env=None, upload_tools=None, with_chaos=None, jes=None,
 
=== modified file 'tests/test_utility.py'
--- tests/test_utility.py	2017-02-25 06:09:40 +0000
+++ tests/test_utility.py	2017-03-01 20:36:24 +0000
@@ -276,11 +276,11 @@
 
     def test_no_args(self):
         cmd_line = []
-        parser = add_basic_testing_arguments(ArgumentParser(), deadline=True)
-        with patch('utility.os.getenv', return_value=''):
-            args = parser.parse_args(cmd_line)
+        parser = add_basic_testing_arguments(ArgumentParser(),
+                                             deadline=True)
+        args = parser.parse_args(cmd_line)
         self.assertEqual(args.env, 'lxd')
-        self.assertEqual(args.juju_bin, '/usr/bin/juju')
+        self.assertEqual(args.juju_bin, None)
 
         self.assertEqual(args.logs, None)
 
@@ -292,14 +292,6 @@
         self.assertEqual(temp_env_name_arg[2:4], ['temp', 'env'])
         self.assertIs(None, args.deadline)
 
-    def test_default_binary(self):
-        cmd_line = []
-        with patch('utility.os.getenv', return_value='/tmp'):
-            with patch('utility.os.path.isfile', return_value=True):
-                parser = add_basic_testing_arguments(ArgumentParser())
-                args = parser.parse_args(cmd_line)
-        self.assertEqual(args.juju_bin, '/tmp/bin/juju')
-
     def test_positional_args(self):
         cmd_line = ['local', '/foo/juju', '/tmp/logs', 'testtest']
         parser = add_basic_testing_arguments(ArgumentParser(), deadline=True)
 
=== modified file 'utility.py'
--- utility.py	2017-02-25 06:09:40 +0000
+++ utility.py	2017-03-01 20:36:24 +0000
@@ -208,6 +208,26 @@
         return 'unknown_test'
 
 
+def generate_default_clean_dir(temp_env_name):
+    """Creates a new unique directory for logging and returns name"""
+    logging.debug('Environment {}'.format(temp_env_name))
+    test_name = temp_env_name.split('-')[0]
+    timestamp = datetime.now().strftime("%Y%m%d%H%M%S")
+    log_dir = os.path.join('/tmp', test_name, 'logs', timestamp)
+
+    try:
+        os.makedirs(log_dir)
+        logging.info('Created logging directory {}'.format(log_dir))
+    except OSError as e:
+        if e.errno == errno.EEXIST:
+            logging.warn('"Directory {} already exists'.format(log_dir))
+        else:
+            raise('Failed to create logging directory: {} ' +
+                  log_dir +
+                  '. Please specify empty folder or try again')
+    return log_dir
+
+
 def _generate_default_temp_env_name():
     """Creates a new unique name for environment and returns the name"""
     # we need to sanitize the name
@@ -216,16 +236,6 @@
     return '{}-{}-temp-env'.format(test_name, timestamp)
 
 
-def _generate_default_binary():
-    """Returns GOPATH juju binary if it exists, otherwise /usr/bin/juju"""
-    if os.getenv('GOPATH'):
-        go_bin = os.getenv('GOPATH') + '/bin/juju'
-        if os.path.isfile(go_bin):
-            return go_bin
-
-    return '/usr/bin/juju'
-
-
 def _to_deadline(timeout):
     return datetime.utcnow() + timedelta(seconds=int(timeout))
 
@@ -233,9 +243,8 @@
 def add_arg_juju_bin(parser):
     parser.add_argument('juju_bin', nargs='?',
                         help='Full path to the Juju binary. By default, this'
-                        ' will use $GOPATH/bin/juju or /usr/bin/juju in that'
-                        ' order.',
-                        default=_generate_default_binary())
+                        ' will use $PATH/juju',
+                        default=None)
 
 
 def add_basic_testing_arguments(parser, using_jes=False, deadline=True,