Merge lp:~nskaggs/juju-ci-tools/add-sane-args-defaults into lp:juju-ci-tools

[Nicholas Skaggs]

Make juju_bin, temp_env_name and logs all as optional arguments with sane defaults

[Nicholas Skaggs (nskaggs)]

Reviewers note; I'm tempted to add a timestamp to the environment name or otherwise make it more unique. Ideas welcome. Would you find that useful or annoying (especially since it may not be the exact same ts as the logging folder)?

[Martin Packman (gz)]

The general idea of having sane defaults for the positional arguments I'm down with, I think we could even default 'env' to 'lxd' or something.

Given this changes add_basic_testing_arguments, TestAddBasicTestingArguments should be changed.

$ rm -rf /tmp/logs
$ touch /tmp/logs
$ make test
...
FAILED (failures=2, errors=37)

Some other comments inline.

review: Needs Fixing

[Aaron Bentley (abentley)]

It seems strange to provide a default for the juju under test. It is, after all, the thing we are testing, and if we don't want to specify it, we might as well not run the test at all.

[Nicholas Skaggs (nskaggs)]

Replies.

[Aaron Bentley (abentley)]

You don't seem to have replied to me.

[Nicholas Skaggs (nskaggs)]

@abentley, I suppose that is the crux of the argument for /usr/bin/juju vs $GOPATH/bin/juju. Odds are you care enough to specify. However, in the case of someone wanting to run the tests locally, they might not particularly care beyond "use the juju I have".

However, I went full bore on Martin's idea of going ahead and removing env too and making a sane default for it. I think the ability to just be able to run our tests is useful. For those of us who will actually attempt to do CI, we'll always want to specify everything, but we're presumably not the primary audience here (other than our localized test writing).

[Aaron Bentley (abentley)]

> However, in the case of
> someone wanting to run the tests locally, they might not particularly care
> beyond "use the juju I have".

Who is that person? Are they a juju developer?

If so, I think they are probably
1. writing a test for a feature they have written, or
2. running a test to make sure their changes don't break it

In both cases, they most likely have a custom binary that they want to test against.

So I still don't see who you're thinking would want to omit the path to the juju binary.

[Nicholas Skaggs (nskaggs)]

> > However, in the case of
> > someone wanting to run the tests locally, they might not particularly care
> > beyond "use the juju I have".
>
> Who is that person? Are they a juju developer?
Anyone who wishes to run tests locally; aka the maas guys, charm guys, openstack team, docs guys, juju developers. Granted ATM, few if any outside of our team run these tests locally.

> If so, I think they are probably
> 1. writing a test for a feature they have written, or
> 2. running a test to make sure their changes don't break it
>
> In both cases, they most likely have a custom binary that they want to test
> against.
I will grant you that indeed that a developer will have a custom binary in either case. Hence I was trying to phrase it as 'use the juju I have'. I won't pretend to know how many binaries developers tend to keep on there system; thus it depends on the developer if we can pick a sane default for them or not. It's likely they'll specify things.

If a developer has the juju binary they are hacking on set in a single place, it would be nice to just use that to enable that workflow, if indeed it exists. That would be an argument for $GOPATH. For the others listed above, I think /usr/bin/juju would be there expectation.

However, even if we assume developers and ourselves will always specify (and I agree that is likely), I do see value in having things just work when executed beyond enabling others. Removing all required arguments does that, so I would argue for adding the default on that basis alone. If we are left with only one argument, and we can reasonably remove it, let's do so. Lowering the barrier to running tests is the goal; albeit without breaking ourselves.

[Aaron Bentley (abentley)]

Additional temp_env_name comment.

[Nicholas Skaggs (nskaggs)]

Addressed uniqueness issue with temp_env_name

[Martin Packman (gz)]

Most changes seem fine, but we still have the test isolation issue from creating the logdir I pointed out before:

ubuntu@go:~/juju-ci-tools$ bzr st
modified:
  tests/test_utility.py
  utility.py
pending merge tips: (use -v to see all merge revisions)
  Nicholas Skaggs 2016-06-07 make temp_env_name truly unique

ubuntu@go:~/juju-ci-tools$ make test
...
FAILED (failures=2, errors=39)

ubuntu@go:~/juju-ci-tools$ bzr st
modified:
  tests/test_utility.py
  utility.py
unknown:
  baz/
  d/
  log/
  log_dir/
  test_utility_20160608144450_logs/
pending merge tips: (use -v to see all merge revisions)
  Nicholas Skaggs 2016-06-07 make temp_env_name truly unique

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

@mgz, I don't get any errors running this. I think you are pointing out I shouldn't be creating a folder during test runs; absolutely correct, so I've patched that out. I also will add some more tests to ensure __clean_dir() works as it should given I've modified it as well. Is there something else you are referring to?

[Martin Packman (gz)]

See my first comment for instructions. Tests should not fail based on the contents on /tmp.

[Nicholas Skaggs (nskaggs)]

I fixed everything in test_utility to use a temp dir instead of a hardcoded path. In a seperate MP I'll fix the rest of trunk with the same fix.

[Nicholas Skaggs (nskaggs)]

Ok, I did this in a simpler way -- only throw an exception if the directory doesn't exist, and we fail to create it. We continue on with warnings otherwise. Tests pass, new tests confirm my expectations.

[Nicholas Skaggs (nskaggs)]

This fixes the /tmp/logs problem by not raising the issue. The only remaining annoyance is -h or --help will create a directory and leave the warning (since the default functions still run). Also, running make test will spam blank folders in /tmp. Comments welcome.

[Christopher Lee (veebers)]

Some comments inline.

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

Replies inline.

[Christopher Lee (veebers)]

Comment re: readability

review: Needs Fixing

[Christopher Lee (veebers)]

Reply to comments

[Christopher Lee (veebers)]

LGTM

review: Approve

[Christopher Lee (veebers)]

> LGTM
Actually I take that back. I have concerns on calling --help creating a directory.

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

Creating a new MP to hash out the --help creating a directory issue. Continuing down the road of abusing things, this works to the extent we don't need to changed the existing tests. Given the constraint of not wanting to change the tests, I'm not sure there's something better. Chris gave the suggestion to just check sys.argv, which is better than my thought of parsing args twice :-)

[lp:~nskaggs/juju-ci-tools/add-sane-args-defaults]

1469. By Nicholas Skaggs on 2016-07-11

fix for bug #1601987

1470. By Nicholas Skaggs on 2016-07-11

fix help test

[Nicholas Skaggs (nskaggs)]

@mgz or @veebers, I'd like to get this landed as there's a bug now, bug #1601987, that needs fixed. Can you review again?

[Martin Packman (gz)]

Sorry Nicholas.

I think the current approach has gone past the point of sanity, we just have to split the parsing from the post-parsing configuration.

Am well aware I am the one who started us down the current path _clean_dir as a cunning hack, but we just should not be doing anything to the environment as a side effect of arg parsing.

[lp:~nskaggs/juju-ci-tools/add-sane-args-defaults]

1471. By Nicholas Skaggs on 2016-07-11

re-merge trunk

1472. By Nicholas Skaggs on 2016-07-12

remove directory creation as part of arg parsing. By default if nothing is specified, we will us the current directory

[Nicholas Skaggs (nskaggs)]

@mgz, are you "ok" with chucking out generating a directory then and just defaulting to something we know exists? I've updated to use cwd, but we could also use temp. If this also doesn't work, I'll make specifying the directory required again and submit.

[lp:~nskaggs/juju-ci-tools/add-sane-args-defaults]

1473. By Nicholas Skaggs on 2016-07-21

move directory creation to bootstrap manager

1474. By Nicholas Skaggs on 2016-07-21

flake8

1475. By Nicholas Skaggs on 2016-07-21

patch out directory creation during no_args test

[Nicholas Skaggs (nskaggs)]

This is ready again for review. Nothing is leftover after running make test, no directories are created when you specify arguments, and no directories are made by running help. This uses bootstrap manager instead of hacking arg parser :-)

[lp:~nskaggs/juju-ci-tools/add-sane-args-defaults]

1476. By Nicholas Skaggs on 2016-07-21

restore _generate_default_clean_dir

[Martin Packman (gz)]

Okay, so this is an improvement, lets land it.

review: Approve

=== modified file 'deploy_stack.py'
--- deploy_stack.py	2016-07-18 17:06:54 +0000
+++ deploy_stack.py	2016-07-21 13:37:06 +0000
@@ -7,6 +7,11 @@
     contextmanager,
     nested,
 )
+from datetime import (
+    datetime,
+)
+
+import errno
 import glob
 import logging
 import os
@@ -487,7 +492,30 @@
             self.known_hosts['0'] = bootstrap_host
 
     @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)
+
         if args.juju_bin == 'FAKE':
             env = SimpleEnvironment.from_config(args.env)
             from tests.test_jujupy import fake_juju_client  # Circular imports

=== modified file 'tests/test_deploy_stack.py'
--- tests/test_deploy_stack.py	2016-07-05 14:15:27 +0000
+++ tests/test_deploy_stack.py	2016-07-21 13:37:06 +0000
@@ -2,6 +2,9 @@
     Namespace,
 )
 from contextlib import contextmanager
+from datetime import (
+    datetime,
+)
 import json
 import logging
 import os
@@ -1034,6 +1037,35 @@
         self.assertEqual(jes_enabled, bs_manager.jes_enabled)
         self.assertEqual({'0': 'example.org'}, bs_manager.known_hosts)
 
+    def test_no_args(self):
+        args = Namespace(
+            env='foo', juju_bin='bar', debug=True, temp_env_name='baz',
+            bootstrap_host='example.org', machine=['example.com'],
+            series='angsty', agent_url='qux', agent_stream='escaped',
+            region='eu-west-northwest-5', logs=None, keep_env=True)
+        with patch('deploy_stack.client_from_config') as fc_mock:
+            with patch('utility.os.makedirs'):
+                bs_manager = BootstrapManager.from_args(args)
+        fc_mock.assert_called_once_with('foo', 'bar', debug=True)
+        self.assertEqual('baz', bs_manager.temp_env_name)
+        self.assertIs(fc_mock.return_value, bs_manager.client)
+        self.assertIs(fc_mock.return_value, bs_manager.tear_down_client)
+        self.assertEqual('example.org', bs_manager.bootstrap_host)
+        self.assertEqual(['example.com'], bs_manager.machines)
+        self.assertEqual('angsty', bs_manager.series)
+        self.assertEqual('qux', bs_manager.agent_url)
+        self.assertEqual('escaped', bs_manager.agent_stream)
+        self.assertEqual('eu-west-northwest-5', bs_manager.region)
+        self.assertIs(True, bs_manager.keep_env)
+        logs_arg = bs_manager.log_dir.split("/")
+        logs_ts = logs_arg[4]
+        self.assertEqual(logs_arg[1:4], ['tmp', 'baz', 'logs'])
+        self.assertTrue(logs_ts, datetime.strptime(logs_ts, "%Y%m%d%H%M%S"))
+        jes_enabled = bs_manager.client.is_jes_enabled.return_value
+        self.assertEqual(jes_enabled, bs_manager.permanent)
+        self.assertEqual(jes_enabled, bs_manager.jes_enabled)
+        self.assertEqual({'0': 'example.org'}, bs_manager.known_hosts)
+
     def test_jes_not_permanent(self):
         with self.assertRaisesRegexp(ValueError, 'Cannot set permanent False'
                                      ' if jes_enabled is True.'):

=== modified file 'tests/test_utility.py'
--- tests/test_utility.py	2016-07-07 21:47:48 +0000
+++ tests/test_utility.py	2016-07-21 13:37:06 +0000
@@ -365,33 +365,26 @@
     def test_no_args(self):
         cmd_line = []
         parser = add_basic_testing_arguments(ArgumentParser())
-        with patch('utility.os.makedirs'):
-            with patch('utility.os.getenv', return_value=''):
-                args = parser.parse_args(cmd_line)
+        with patch('utility.os.getenv', return_value=''):
+            args = parser.parse_args(cmd_line)
         self.assertEqual(args.env, 'lxd')
         self.assertEqual(args.juju_bin, '/usr/bin/juju')
 
-        logs_arg = args.logs.split("/")
-        logs_ts = logs_arg[4]
-        self.assertEqual(logs_arg[1:4], ['tmp', 'test_utility', 'logs'])
-        self.assertTrue(logs_ts, datetime.strptime(logs_ts, "%Y%m%d%H%M%S"))
+        self.assertEqual(args.logs, None)
 
-        temp_env_name_arg = args.temp_env_name.split("_")
-        temp_env_name_ts = temp_env_name_arg[2]
-        self.assertEqual(temp_env_name_arg[0:2], ['test', 'utility'])
+        temp_env_name_arg = args.temp_env_name.split("-")
+        temp_env_name_ts = temp_env_name_arg[1]
+        self.assertEqual(temp_env_name_arg[0:1], ['testutility'])
         self.assertTrue(temp_env_name_ts,
                         datetime.strptime(temp_env_name_ts, "%Y%m%d%H%M%S"))
-        self.assertEqual(temp_env_name_arg[3:5], ['temp', 'env'])
-
-        self.assertEqual(logs_ts, temp_env_name_ts)
+        self.assertEqual(temp_env_name_arg[2:4], ['temp', 'env'])
 
     def test_default_binary(self):
         cmd_line = []
-        with patch('utility.os.makedirs'):
-            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)
+        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):
@@ -441,6 +434,18 @@
             self.assertEqual(warned, [])
         self.assertEqual("", self.log_stream.getvalue())
 
+    def test_no_warn_on_help(self):
+        """Special case help should not generate a warning"""
+        with warnings.catch_warnings(record=True) as warned:
+            with patch('utility.sys.exit'):
+                parser = add_basic_testing_arguments(ArgumentParser())
+                cmd_line = ['-h']
+                parser.parse_args(cmd_line)
+                cmd_line = ['--help']
+                parser.parse_args(cmd_line)
+
+            self.assertEqual(warned, [])
+
     def test_warn_on_nonexistent_directory_creation(self):
         with warnings.catch_warnings(record=True) as warned:
             log_dir = mkdtemp()
@@ -454,25 +459,6 @@
                 r"Not a directory " + log_dir)
             self.assertEqual("", self.log_stream.getvalue())
 
-    def test_warn_on_directory_creation_failure(self):
-        with warnings.catch_warnings(record=True) as warned:
-            with patch('utility.os.makedirs', side_effect=OSError):
-                cmd_line = []
-                try:
-                    parser = add_basic_testing_arguments(ArgumentParser())
-                    parser.parse_args(cmd_line)
-                except OSError:
-                    # we catch our thrown OSError
-                    pass
-                else:
-                    self.fail('No exception thrown after' +
-                              ' directory creation failure')
-                self.assertEqual(len(warned), 1)
-                self.assertRegexpMatches(
-                    str(warned[0].message),
-                    r"Failed to create logging directory: /tmp/test_utility/" +
-                    "logs/.*. Please specify empty folder or try again")
-
     def test_debug(self):
         cmd_line = ['local', '/foo/juju', '/tmp/logs', 'testtest', '--debug']
         parser = add_basic_testing_arguments(ArgumentParser())

=== modified file 'utility.py'
--- utility.py	2016-07-07 22:00:51 +0000
+++ utility.py	2016-07-21 13:37:06 +0000
@@ -300,27 +300,12 @@
         return 'unknown_test'
 
 
-def _generate_default_clean_dir(timestamp):
-    """Creates a new unique directory for logging and returns the name"""
-    log_dir = os.path.join('/tmp', _get_test_name_from_filename(),
-                           'logs', timestamp)
-    try:
-        os.makedirs(log_dir)
-        warnings.warn('"Created logging directory {}'.format(log_dir))
-    except OSError as e:
-        if e.errno == errno.EEXIST:
-            pass
-        else:
-            warnings.warn('Failed to create logging directory: ' +
-                          log_dir +
-                          ". Please specify empty folder or try again")
-            raise
-    return log_dir
-
-
-def _generate_default_temp_env_name(timestamp):
+def _generate_default_temp_env_name():
     """Creates a new unique name for environment and returns the name"""
-    return '{}_{}_temp_env'.format(_get_test_name_from_filename(), timestamp)
+    # we need to sanitize the name
+    timestamp = datetime.now().strftime("%Y%m%d%H%M%S")
+    test_name = re.sub('[^a-zA-Z]', '', _get_test_name_from_filename())
+    return '{}-{}-temp-env'.format(test_name, timestamp)
 
 
 def _generate_default_binary():
@@ -357,8 +342,6 @@
     """
 
     # Optional postional arguments
-    # generate timestamp for use with default args
-    timestamp = datetime.now().strftime("%Y%m%d%H%M%S")
     parser.add_argument(
         'env', nargs='?',
         help='The juju environment to base the temp test environment on.',
@@ -370,16 +353,14 @@
                         default=_generate_default_binary())
     parser.add_argument('logs',  nargs='?',  type=_clean_dir,
                         help='A directory in which to store logs. By default,'
-                        ' this will generate a directory named after the test'
-                        ' and under /tmp, with a timestamp of the run. '
-                        ' /tmp/test_name/logs/timestamp',
-                        default=_generate_default_clean_dir(timestamp))
+                        ' this will use the current directory',
+                        default=None)
     parser.add_argument('temp_env_name', nargs='?',
                         help='A temporary test environment name. By default, '
                         ' this will generate an enviroment name using the '
                         ' timestamp and testname. '
                         ' test_name_timestamp_temp_env',
-                        default=_generate_default_temp_env_name(timestamp))
+                        default=_generate_default_temp_env_name())
 
     # Optional keyword arguments.
     parser.add_argument('--debug', action='store_true',