Merge lp:~andrewjbeach/juju-ci-tools/bootstrap-args into lp:juju-ci-tools

[Andrew James Beach]

Added a new parameter to get_bootstrap_args: agent_version. It also appears
in the bootstrap methods so you can pass in the value.

This conficts with upload_tools, if both are given an exception is raised.

Modified get_bootstrap_args and bootstrap methods in EnvJujuClient.
Added tests for the new parameter in test_jujupy.py.

[Aaron Bentley (abentley)]

I don't see any handling for juju 1.x. Did you test it?

Since EnvJujuClient's bootstrap implementation is used by EnvJujuClient1X, but its get_bootstrap_args has not been updated, I expect to fail because too many parameters are supplied.

I would expect EnvJujuClient1X.get_bootstrap_args to:
1. always have a signature matching EnvJujuClient.get_bootstrap_args, so it would accept agent_version, defaulted to None (like EnvJujuClient2B2.get_bootstrap_args)
2. raise an exception (e.g. ValueError) if agent_version is not None.

review: Needs Information

[lp:~andrewjbeach/juju-ci-tools/bootstrap-args]

1638. By Andrew James Beach on 2016-10-04

Added checks to make sure the versions that don't support the new bootstrap arguments don't get them.

[Andrew James Beach (andrewjbeach)]

EnvJujuClient1X doesn't use EnvJujuClient's bootstrap, it uses the one in from 2A2. I instead did that to 2B2, which does use EnvJujuClient's bootstrap. If you think it is important I could repeat the process on other versions.

[Aaron Bentley (abentley)]

Thanks for explaining. Sorry I missed the EnvJujuClient2A2 implementation. Ideally that would have the same signature as the EnvJujuClient implementation, but it seems to have diverged considerably without causing actual problems, so I guess we don't need to address that right now.

Aside from a couple of typos, this is good to land. I do have a couple of suggestions for the get_bootstrap_args logic, but they're not required.

review: Approve

[lp:~andrewjbeach/juju-ci-tools/bootstrap-args]

1639. By Andrew James Beach on 2016-10-04

Last bit of pollish.

=== modified file 'jujupy.py'
--- jujupy.py	2016-10-03 13:12:25 +0000
+++ jujupy.py	2016-10-04 18:45:33 +0000
@@ -1143,9 +1143,10 @@
             return cloud
         return '{}/{}'.format(cloud, region)
 
-    def get_bootstrap_args(self, upload_tools, config_filename,
-                           bootstrap_series=None, credential=None,
-                           auto_upgrade=False, metadata_source=None, to=None):
+    def get_bootstrap_args(
+            self, upload_tools, config_filename, bootstrap_series=None,
+            credential=None, auto_upgrade=False, metadata_source=None,
+            to=None, agent_version=None):
         """Return the bootstrap arguments for the substrate."""
         if self.env.joyent:
             # Only accept kvm packages by requiring >1 cpu core, see lp:1446264
@@ -1158,9 +1159,14 @@
                 cloud_region, '--config', config_filename,
                 '--default-model', self.env.environment]
         if upload_tools:
+            if agent_version is not None:
+                raise ValueError(
+                    'agent-version may not be given with upload-tools.')
             args.insert(0, '--upload-tools')
         else:
-            args.extend(['--agent-version', self.get_matching_agent_version()])
+            if agent_version is None:
+                agent_version = self.get_matching_agent_version()
+            args.extend(['--agent-version', agent_version])
         if bootstrap_series is not None:
             args.extend(['--bootstrap-series', bootstrap_series])
         if credential is not None:
@@ -1236,13 +1242,13 @@
 
     def bootstrap(self, upload_tools=False, bootstrap_series=None,
                   credential=None, auto_upgrade=False, metadata_source=None,
-                  to=None):
+                  to=None, agent_version=None):
         """Bootstrap a controller."""
         self._check_bootstrap()
         with self._bootstrap_config() as config_filename:
             args = self.get_bootstrap_args(
                 upload_tools, config_filename, bootstrap_series, credential,
-                auto_upgrade, metadata_source, to)
+                auto_upgrade, metadata_source, to, agent_version)
             self.update_user_name()
             self.juju('bootstrap', args, include_e=False)
 
@@ -2296,10 +2302,20 @@
 
 class EnvJujuClient2B2(EnvJujuClient2B3):
 
-    def get_bootstrap_args(self, upload_tools, config_filename,
-                           bootstrap_series=None, credential=None,
-                           auto_upgrade=False, metadata_source=None, to=None):
+    def get_bootstrap_args(
+            self, upload_tools, config_filename, bootstrap_series=None,
+            credential=None, auto_upgrade=False, metadata_source=None,
+            to=None, agent_version=None):
         """Return the bootstrap arguments for the substrate."""
+        err_fmt = 'EnvJujuClient2B2 does not support bootstrap argument {}'
+        if auto_upgrade:
+            raise ValueError(err_fmt.format('auto_upgrade'))
+        if metadata_source is not None:
+            raise ValueError(err_fmt.format('metadata_source'))
+        if to is not None:
+            raise ValueError(err_fmt.format('to'))
+        if agent_version is not None:
+            raise ValueError(err_fmt.format('agent_version'))
         if self.env.joyent:
             # Only accept kvm packages by requiring >1 cpu core, see lp:1446264
             constraints = 'mem=2G cpu-cores=1'

=== modified file 'tests/test_jujupy.py'
--- tests/test_jujupy.py	2016-10-03 13:12:25 +0000
+++ tests/test_jujupy.py	2016-10-04 18:45:33 +0000
@@ -1046,6 +1046,24 @@
             '--config', 'config', '--default-model', 'foo',
             '--bootstrap-series', 'angsty'))
 
+    def test_get_bootstrap_args_agent_version(self):
+        env = JujuData('foo', {'type': 'bar', 'region': 'baz'})
+        client = EnvJujuClient(env, '2.0-zeta1', None)
+        args = client.get_bootstrap_args(upload_tools=False,
+                                         config_filename='config',
+                                         agent_version='2.0-lambda1')
+        self.assertEqual(('--constraints', 'mem=2G', 'foo', 'bar/baz',
+                          '--config', 'config', '--default-model', 'foo',
+                          '--agent-version', '2.0-lambda1'), args)
+
+    def test_get_bootstrap_args_upload_tools_and_agent_version(self):
+        env = JujuData('foo', {'type': 'bar', 'region': 'baz'})
+        client = EnvJujuClient(env, '2.0-zeta1', None)
+        with self.assertRaises(ValueError):
+            client.get_bootstrap_args(upload_tools=True,
+                                      config_filename='config',
+                                      agent_version='2.0-lambda1')
+
     def test_add_model_hypenated_controller(self):
         self.do_add_model(
             'kill-controller', 'add-model', ('-c', 'foo'))
@@ -3553,6 +3571,21 @@
             '--upload-tools', '--constraints', 'mem=2G', 'foo', 'bar/baz',
             '--config', 'config', '--bootstrap-series', 'angsty'))
 
+    def test_get_bootstrap_args_reject_new_args(self):
+        env = JujuData('foo', {'type': 'bar', 'region': 'baz'})
+        client = EnvJujuClient2B2(env, '2.0-zeta1', None)
+        base_args = {'upload_tools': True,
+                     'config_filename': 'config',
+                     'bootstrap_series': 'angsty'}
+        with self.assertRaises(ValueError):
+            client.get_bootstrap_args(auto_upgrade=True, **base_args)
+        with self.assertRaises(ValueError):
+            client.get_bootstrap_args(metadata_source='/foo', **base_args)
+        with self.assertRaises(ValueError):
+            client.get_bootstrap_args(to='cur', **base_args)
+        with self.assertRaises(ValueError):
+            client.get_bootstrap_args(agent_version='1.0.0', **base_args)
+
     def test_bootstrap_upload_tools(self):
         env = JujuData('foo', {'type': 'foo', 'region': 'baz'})
         client = EnvJujuClient2B2(env, '2.0-zeta1', None)