Merge lp:~rvb/maas-test/fix-proxy-usage into lp:maas-test

[Raphaël Badin]

This branch uses the proxy URL in maasfixture: if it is defined, MAAS' http_proxy option is set.

Drive-by fix: add a unit test for configure_default_series (named ­— unexpectedly —test_configure_default_series_configures_series()).

[Gavin Panella (allenap)]

Looks good.

[1]

-    def configure_default_series(self, series):
-        """Set the default series used for enlistment and commissioning."""
+    def configure_maas(self, name, value):
+        """Set a config value in MAAS."""

The name "configure_maas" is somewhat tautological. The full name of
this function is: maastest.maasfixture.MAASFixture.configure_maas. I
think it's safe to call it just "configure".

[2]

         if response.code != httplib.OK:
             # TODO: include the response's content in the exception
             # message, here and in other places in this file.
-            raise Exception("Error configuring the commissiong series")
+            raise Exception("Error configuring '%s'" % name)

As discussed in a previous branch, we really need to do something about
this. It's hiding what might be very useful information that could save
hours of debugging.

[3]

+        self.patch(maas_fixture, 'admin_maas_client', mock.MagicMock())
+        series = self.getUniqueString()
+        call_mock = self.patch_MAASClient(
+            maas_fixture.admin_maas_client, 'post', '')

Do you need the first line here?

[4]

+        self.assertEqual(
+            [mock.call(
+                '/api/1.0/maas/', op='set_config',
+                name='commissioning_distro_series', value=series)],
+            (call_mock.mock_calls))

The braces in the last line are superflous, though harmless.

review: Approve

[lp:~rvb/maas-test/fix-proxy-usage]

70. By Raphaël Badin on 2013-11-26

Review fixes.

[Raphaël Badin (rvb)]

> Looks good.
>
>
> [1]
>
> -    def configure_default_series(self, series):
> -        """Set the default series used for enlistment and commissioning."""
> +    def configure_maas(self, name, value):
> +        """Set a config value in MAAS."""
>
> The name "configure_maas" is somewhat tautological. The full name of
> this function is: maastest.maasfixture.MAASFixture.configure_maas. I
> think it's safe to call it just "configure".

Right, fixed.

> [2]
>
>         if response.code != httplib.OK:
>             # TODO: include the response's content in the exception
>             # message, here and in other places in this file.
> -            raise Exception("Error configuring the commissiong series")
> +            raise Exception("Error configuring '%s'" % name)
>
> As discussed in a previous branch, we really need to do something about
> this. It's hiding what might be very useful information that could save
> hours of debugging.

Yeah, I know that's why I added the todo. But let's fix that in another branch.

>
>
> [3]
>
> +        self.patch(maas_fixture, 'admin_maas_client', mock.MagicMock())
> +        series = self.getUniqueString()
> +        call_mock = self.patch_MAASClient(
> +            maas_fixture.admin_maas_client, 'post', '')
>
> Do you need the first line here?

Yes we do, because maas_fixture.admin_maas_client is not created in the context of this test.

> [4]
>
> +        self.assertEqual(
> +            [mock.call(
> +                '/api/1.0/maas/', op='set_config',
> +                name='commissioning_distro_series', value=series)],
> +            (call_mock.mock_calls))
>
> The braces in the last line are superflous, though harmless.

Fixed.

Thanks for the review!

=== modified file 'maastest/maasfixture.py'
--- maastest/maasfixture.py	2013-11-26 13:50:33 +0000
+++ maastest/maasfixture.py	2013-11-26 22:46:23 +0000
@@ -181,16 +181,23 @@
             check_call=True)
         logging.info("Done importing MAAS PXE Files.")
 
-    def configure_default_series(self, series):
-        """Set the default series used for enlistment and commissioning."""
+    def configure(self, name, value):
+        """Set a config value in MAAS."""
         uri = utils.get_uri('maas/')
         response = self.admin_maas_client.post(
-            uri, op='set_config', name='commissioning_distro_series',
-            value=series)
+            uri, op='set_config', name=name, value=value)
         if response.code != httplib.OK:
             # TODO: include the response's content in the exception
             # message, here and in other places in this file.
-            raise Exception("Error configuring the commissiong series")
+            raise Exception("Error configuring '%s'" % name)
+
+    def configure_default_series(self, series):
+        """Set the default series used for enlistment and commissioning."""
+        self.configure('commissioning_distro_series', series)
+
+    def configure_http_proxy(self, proxy_url):
+        """Set the proxy to be used by MAAS."""
+        self.configure('http_proxy', proxy_url)
 
     def setUp(self):
         """Install and configure MAAS inside the virtual machine.
@@ -212,6 +219,8 @@
         self.admin_api_key = api_key
         self.admin_maas_client = maas_client
         # MAAS setup.
+        if self.proxy_url:  # None or '' indicates no proxy.
+            self.configure_http_proxy(self.proxy_url)
         self.import_maas_images(
             series=self.series, architecture=self.architecture)
         self.configure_default_series(self.series)

=== modified file 'maastest/tests/test_maasfixture.py'
--- maastest/tests/test_maasfixture.py	2013-11-26 13:20:28 +0000
+++ maastest/tests/test_maasfixture.py	2013-11-26 22:46:23 +0000
@@ -423,6 +423,38 @@
                 maas_fixture.collect_logs.mock_calls,
             ])
 
+    def test_configure_default_series_configures_series(self):
+        fixture = self.make_kvm_fixture()
+        maas_fixture = self.make_maas_fixture(fixture)
+        self.patch(maas_fixture, 'admin_maas_client', mock.MagicMock())
+        series = self.getUniqueString()
+        call_mock = self.patch_MAASClient(
+            maas_fixture.admin_maas_client, 'post', '')
+
+        maas_fixture.configure_default_series(series)
+
+        self.assertEqual(
+            [mock.call(
+                '/api/1.0/maas/', op='set_config',
+                name='commissioning_distro_series', value=series)],
+            call_mock.mock_calls)
+
+    def test_configure_http_proxy_configure_http_proxy(self):
+        fixture = self.make_kvm_fixture()
+        proxy_url = self.getUniqueString()
+        maas_fixture = self.make_maas_fixture(fixture, proxy_url=proxy_url)
+        self.patch(maas_fixture, 'admin_maas_client', mock.MagicMock())
+        call_mock = self.patch_MAASClient(
+            maas_fixture.admin_maas_client, 'post', '')
+
+        maas_fixture.configure_http_proxy(proxy_url)
+
+        self.assertEqual(
+            [mock.call(
+                '/api/1.0/maas/', op='set_config',
+                name='http_proxy', value=proxy_url)],
+            (call_mock.mock_calls))
+
     def test_setUp_calls_methods_if_direct_ip_set(self):
         fixture = self.make_kvm_fixture()
         fixture.direct_ip = '127.11.24.38'
@@ -442,6 +474,8 @@
         self.patch(
             maas_fixture, 'configure_cluster', mock.MagicMock())
         self.patch(
+            maas_fixture, 'configure_http_proxy', mock.MagicMock())
+        self.patch(
             maas_fixture, 'import_maas_images', mock.MagicMock())
 
         with maas_fixture:
@@ -452,6 +486,7 @@
                 [mock.call()],
                 [mock.call()],
                 [mock.call()],
+                [mock.call(maas_fixture.proxy_url)],
                 [mock.call(
                     series=maas_fixture.series,
                     architecture=maas_fixture.architecture)],
@@ -460,9 +495,28 @@
                 maas_fixture.configure_default_maas_url.mock_calls,
                 maas_fixture.check_cluster_connected.mock_calls,
                 maas_fixture.configure_cluster.mock_calls,
+                maas_fixture.configure_http_proxy.mock_calls,
                 maas_fixture.import_maas_images.mock_calls,
             ])
 
+    def test_setUp_does_not_configure_proxy_if_no_proxy(self):
+        fixture = self.make_kvm_fixture()
+        maas_fixture = self.make_maas_fixture(fixture, proxy_url='')
+        self.patch(
+            maas_fixture, 'query_api_key',
+            mock.MagicMock(return_value='fake:api:key'))
+        self.patch(
+            maas_fixture, 'configure_http_proxy', mock.MagicMock())
+
+        with maas_fixture:
+            pass
+
+        self.assertEqual(
+            [[]],
+            [
+                maas_fixture.configure_http_proxy.mock_calls,
+            ])
+
     def test_setUp_does_not_call_methods_if_direct_ip_unset(self):
         fixture = self.make_kvm_fixture()
         maas_fixture = self.make_maas_fixture(fixture)