Merge lp:~blake-rouse/maas/fix-1364062 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Set http_proxy and https_proxy when importing boot resources on the region.

[Raphaël Badin (rvb)]

Looks good!

review: Approve

[Andres Rodriguez (andreserl)]

we really need to test this and how things are affected...

On Tue, Sep 2, 2014 at 11:40 AM, <email address hidden> wrote:

> The proposal to merge lp:~blake-rouse/maas/fix-1364062 into lp:maas has
> been updated.
>
> Status: Approved => Merged
>
> For more details, see:
> https://code.launchpad.net/~blake-rouse/maas/fix-1364062/+merge/233070
> --
> https://code.launchpad.net/~blake-rouse/maas/fix-1364062/+merge/233070
> You are subscribed to branch lp:maas.
>

--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer

[Blake Rouse (blake-rouse)]

Are you saying the config value is only for the clusters and the nodes, not for the region?

I could see that assumption. In that case the server should just be setup to use that proxy. I have nothing against either.

[Julian Edwards (julian-edwards)]

On Tuesday 02 September 2014 14:46:11 you wrote:
> You can probably do the check in one go:
> self.assertEqual(
> (proxy_address, proxy_address),
> (capture.env['http_proxy'], capture.env['http_proxys']))

ARGH. Please stop doing this, it makes test errors almost unreadable.

Use self.expectThat, which delays errors until the test is finished.

In this instance you can also use MatchesAll as you have the same predicate:

self.assertThat(
    proxy_address, MatchesAll(
        Equals(capture.env['http_proxy']),
        Equals(capture.env['https_proxy'])
    ))

[Julian Edwards (julian-edwards)]

On Tuesday 02 September 2014 16:12:40 you wrote:
> we really need to test this and how things are affected...

Ideally, before landing run the branch through the CI using the instructions
that rvb provided.

=== modified file 'src/maasserver/bootresources.py'
--- src/maasserver/bootresources.py	2014-09-02 08:36:29 +0000
+++ src/maasserver/bootresources.py	2014-09-02 15:28:52 +0000
@@ -43,6 +43,7 @@
     BootResourceSet,
     BootSource,
     BootSourceSelection,
+    Config,
     LargeFile,
     NodeGroup,
     )
@@ -880,6 +881,10 @@
         variables = {
             'GNUPGHOME': get_maas_user_gpghome(),
             }
+        http_proxy = Config.objects.get_config('http_proxy')
+        if http_proxy is not None:
+            variables['http_proxy'] = http_proxy
+            variables['https_proxy'] = http_proxy
         with environment_variables(variables):
             maaslog.info("Started importing of boot resources.")
             sources = [source.to_dict() for source in BootSource.objects.all()]

=== modified file 'src/maasserver/tests/test_bootresources.py'
--- src/maasserver/tests/test_bootresources.py	2014-09-02 06:21:04 +0000
+++ src/maasserver/tests/test_bootresources.py	2014-09-02 15:28:52 +0000
@@ -45,6 +45,7 @@
     BootResourceSet,
     BootSource,
     BootSourceSelection,
+    Config,
     LargeFile,
     )
 from maasserver.testing.factory import factory
@@ -975,6 +976,18 @@
 
 class TestImportImages(MAASTestCase):
 
+    def patch_and_capture_env_for_download_all_boot_resources(self):
+        class CaptureEnv:
+            """Fake function; records a copy of the environment."""
+
+            def __call__(self, *args, **kwargs):
+                self.args = args
+                self.env = environ.copy()
+
+        capture = self.patch(
+            bootresources, 'download_all_boot_resources', CaptureEnv())
+        return capture
+
     def test_download_boot_resources_syncs_repo(self):
         fake_sync = self.patch(bootresources.BootResourceRepoWriter, 'sync')
         store = BootResourceStore()
@@ -1082,21 +1095,29 @@
         descriptions.is_empty.return_value = False
         fake_image_descriptions.return_value = descriptions
         self.patch(bootresources, 'map_products')
-
-        class CaptureEnv:
-            """Fake function; records a copy of the environment."""
-
-            def __call__(self, *args, **kwargs):
-                self.args = args
-                self.env = environ.copy()
-
-        fake_download = self.patch(
-            bootresources, 'download_all_boot_resources', CaptureEnv())
+        capture = self.patch_and_capture_env_for_download_all_boot_resources()
 
         bootresources._import_resources(force=True)
         self.assertEqual(
             bootresources.get_maas_user_gpghome(),
-            fake_download.env['GNUPGHOME'])
+            capture.env['GNUPGHOME'])
+
+    def test__import_resources_has_env_http_and_https_proxy_set(self):
+        proxy_address = factory.make_name('proxy')
+        Config.objects.set_config('http_proxy', proxy_address)
+
+        fake_image_descriptions = self.patch(
+            bootresources, 'download_all_image_descriptions')
+        descriptions = Mock()
+        descriptions.is_empty.return_value = False
+        fake_image_descriptions.return_value = descriptions
+        self.patch(bootresources, 'map_products')
+        capture = self.patch_and_capture_env_for_download_all_boot_resources()
+
+        bootresources._import_resources(force=True)
+        self.assertEqual(
+            (proxy_address, proxy_address),
+            (capture.env['http_proxy'], capture.env['http_proxy']))
 
     def test__import_resources_calls_import_boot_images_on_clusters(self):
         nodegroup = MagicMock()