Merge lp:~rvb/maas-test/fix-cache-config into lp:maas-test

[Raphaël Badin]

Configure the proxy to actually cache things.

By default, the proxy was configured to cache things in /var/cache/polipo… which, obviously, didn't work at all since the proxy is not run as root. This branch fixes that by configuring the proxy to write its cache into '~/.maas-test/proxy_cache/'.

[Gavin Panella (allenap)]

Looks good. A couple of things need fixing, but there's no need to block
landing once they're done.

[1]

+# TODO: '.maas-test' shouln't be harcoded like that, it should
+# be a parameter passed to LocalProxyFixture().
+DISK_CACHE_ROOT = '~/.maas-test/proxy_cache/'

Or refer to the XDG spec, and use XDG_CACHE_HOME. There's an xdg module
which you could use like so:

   from xdg.BaseDirectory import save_cache_path
   DISK_CACHE_ROOT = save_cache_path("maas-test", "proxy-cache")

See the xdg.BaseDirectory module source for more.

The packages are python-xdg and python3-xdg.

[2]

+    def create_cache_root(self):
+        cache_root = os.path.expanduser(DISK_CACHE_ROOT)
+        try:
+            os.makedirs(cache_root)
+        except OSError:
+            # Directory already exists.
+            pass

Please check for os.EEXIST:

        try:
            os.makedirs(cache_root)
        except OSError as error:
            if error.code != errno.EEXIST:
                raise

review: Approve

[lp:~rvb/maas-test/fix-cache-config]

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

Review fixes.

[Raphaël Badin (rvb)]

> Looks good. A couple of things need fixing, but there's no need to block
> landing once they're done.
>
>
> [1]
>
> +# TODO: '.maas-test' shouln't be harcoded like that, it should
> +# be a parameter passed to LocalProxyFixture().
> +DISK_CACHE_ROOT = '~/.maas-test/proxy_cache/'
>
> Or refer to the XDG spec, and use XDG_CACHE_HOME. There's an xdg module
> which you could use like so:
>
>   from xdg.BaseDirectory import save_cache_path
>   DISK_CACHE_ROOT = save_cache_path("maas-test", "proxy-cache")
>
> See the xdg.BaseDirectory module source for more.
>
> The packages are python-xdg and python3-xdg.

Not sure what the benefit would be… plus it would need another dependency. I won't change it now but if you think it's worth it and explain to me why, I'm happy to fix it later.
>
>
> [2]
>
> +    def create_cache_root(self):
> +        cache_root = os.path.expanduser(DISK_CACHE_ROOT)
> +        try:
> +            os.makedirs(cache_root)
> +        except OSError:
> +            # Directory already exists.
> +            pass
>
> Please check for os.EEXIST:
>
>        try:
>            os.makedirs(cache_root)
>        except OSError as error:
>            if error.code != errno.EEXIST:
>                raise

Arg! You're right. Fixed. (The fact that Python forces us to do that is really ugly…)

[Gavin Panella (allenap)]

> > [1]
> >
> > +# TODO: '.maas-test' shouln't be harcoded like that, it should
> > +# be a parameter passed to LocalProxyFixture().
> > +DISK_CACHE_ROOT = '~/.maas-test/proxy_cache/'
> >
> > Or refer to the XDG spec, and use XDG_CACHE_HOME. There's an xdg module
> > which you could use like so:
> >
> >   from xdg.BaseDirectory import save_cache_path
> >   DISK_CACHE_ROOT = save_cache_path("maas-test", "proxy-cache")
> >
> > See the xdg.BaseDirectory module source for more.
> >
> > The packages are python-xdg and python3-xdg.
>
> Not sure what the benefit would be… plus it would need another
> dependency. I won't change it now but if you think it's worth it and
> explain to me why, I'm happy to fix it later.

I think it's The Right Thing To Do, basically, in the absence of
anything better. It means that people know which directories can be
safely left out of backups, which can be purged when space gets short,
and so on. It's what I ought to have used for maas-cli.

I wouldn't worry about the dependency at all: python-xdg is almost
certain to be installed already, and it's in main. Dependencies are not
evil either. As a team we seem to be slipping into that mindset. They're
tricky now as we transition to Python 3, but the reason stuff is
packaged is so it can be used! We should depend on anything we need.

Fwiw, save_cache_path() also creates the directory, which would save you
the makedirs shenanigans.

[Julian Edwards (julian-edwards)]

On Tuesday 26 Nov 2013 23:02:48 you wrote:
> I wouldn't worry about the dependency at all: python-xdg is almost
> certain to be installed already, and it's in main. Dependencies are not
> evil either. As a team we seem to be slipping into that mindset. They're
> tricky now as we transition to Python 3, but the reason stuff is
> packaged is so it can be used! We should depend on anything we need.

FWIW I don't think we are slipping into that mindset, but we are looking to
reduce *unnecessary* dependencies as they create an additional maintenance and
security burden.

=== modified file 'maastest/proxyfixture.py'
--- maastest/proxyfixture.py	2013-11-26 13:50:33 +0000
+++ maastest/proxyfixture.py	2013-11-26 22:44:24 +0000
@@ -21,9 +21,14 @@
 from fixtures import Fixture
 from maastest import utils
 from netaddr import IPNetwork
+import errno
 import netifaces
 
 
+# TODO: '.maas-test' shouln't be harcoded like that, it should
+# be a parameter passed to LocalProxyFixture().
+DISK_CACHE_ROOT = '~/.maas-test/proxy_cache/'
+
 PROXY_CONFIG_ITEMS = {
     'daemonise': 'true',
     'logFile': '%(config_dir)s/proxy.log',
@@ -31,6 +36,7 @@
     'proxyAddress': "::0",
     'proxyPort': '%(port)d',
     'allowedClients': '127.0.0.1,%(ip_address)s/%(netmask)s',
+    'diskCacheRoot': DISK_CACHE_ROOT
     }
 
 IP_ADDRESS_RE = re.compile(
@@ -66,6 +72,8 @@
         """
         super(LocalProxyFixture, self).__init__()
         self.port = port
+        # TODO: '.maas-test' shouln't be harcoded like that, it should
+        # be a parameter passed to LocalProxyFixture().
         self.config_dir = os.path.join(
             os.path.expanduser('~'), '.maas-test')
         self.pid_file = os.path.join(self.config_dir, 'proxy.pid')
@@ -107,6 +115,11 @@
     def setUp(self):
         """Configure the polipo caching http proxy."""
         super(LocalProxyFixture, self).setUp()
+        self.create_cache_root()
+        self.start_proxy()
+        self.addCleanup(self.kill_running_proxy)
+
+    def start_proxy(self):
         logging.info("Checking for running proxy instance...")
         if os.path.exists(self.pid_file):
             raise Exception(
@@ -115,9 +128,17 @@
         logging.info("Starting proxy...")
         utils.run_command(
             ['polipo'] + self._get_config_arguments(), check_call=True)
-        self.addCleanup(self.kill_running_proxy)
         logging.info("Done starting proxy.")
 
+    def create_cache_root(self):
+        cache_root = os.path.expanduser(DISK_CACHE_ROOT)
+        try:
+            os.makedirs(cache_root)
+        except OSError as error:
+            if error.errno != errno.EEXIST:
+                raise
+            # Directory already exists.
+
     def kill_running_proxy(self):
         """Kill a running proxy."""
         logging.info("Killing proxy...")

=== modified file 'maastest/tests/test_proxyfixture.py'
--- maastest/tests/test_proxyfixture.py	2013-11-26 17:22:35 +0000
+++ maastest/tests/test_proxyfixture.py	2013-11-26 22:44:24 +0000
@@ -18,8 +18,15 @@
 
 from maastest import utils
 from maastest.proxyfixture import LocalProxyFixture
+from maastest import proxyfixture
 import mock
 import netifaces
+<<<<<<< TREE
+=======
+import random
+from fixtures import TempDir
+from testtools.matchers import DirExists
+>>>>>>> MERGE-SOURCE
 import testtools
 
 
@@ -56,17 +63,53 @@
             "http://192.168.0.1:%s" % port,
             proxy_fixture.get_url())
 
-    def test_set_up_runs_polipo_with_config(self):
+    def test_set_up_calls_all_setup_methods(self):
+        self.patch(LocalProxyFixture, 'create_cache_root', mock.MagicMock())
+        self.patch(LocalProxyFixture, 'start_proxy', mock.MagicMock())
+
+        random_port = self.get_random_port_number()
+        with LocalProxyFixture(port=random_port) as fixture:
+            pass
+
+        self.assertEqual(
+            [[mock.call()], [mock.call()]],
+            [
+                fixture.create_cache_root.mock_calls,
+                fixture.start_proxy.mock_calls,
+            ])
+
+    def test_start_proxy_starts_proxy(self):
         mock_run_command = mock.MagicMock()
         self.patch(utils, 'run_command', mock_run_command)
         proxy_fixture = LocalProxyFixture(port=self.get_random_port_number())
-        proxy_fixture.setUp()
+
+        proxy_fixture.start_proxy()
+
         self.assertEqual(
             mock.call(
                 ['polipo'] + proxy_fixture._get_config_arguments(),
                 check_call=True),
             mock_run_command.mock_calls[-1])
 
+    def test_create_cache_root_creates_directory(self):
+        temp_dir = self.useFixture(TempDir()).path
+        test_cache_path = temp_dir + "path"
+        self.patch(proxyfixture, 'DISK_CACHE_ROOT', test_cache_path)
+        proxy_fixture = LocalProxyFixture(port=self.get_random_port_number())
+
+        proxy_fixture.create_cache_root()
+
+        self.assertThat(test_cache_path, DirExists())
+
+    def test_create_cache_root_ignores_existing_directory(self):
+        temp_dir = self.useFixture(TempDir()).path
+        self.patch(proxyfixture, 'DISK_CACHE_ROOT', temp_dir)
+        proxy_fixture = LocalProxyFixture(port=self.get_random_port_number())
+
+        proxy_fixture.create_cache_root()
+
+        self.assertThat(temp_dir, DirExists())
+
     def test_get_network_address(self):
         proxy_fixture = LocalProxyFixture(port=self.get_random_port_number())
         proxy_adddress = proxy_fixture._get_network_address()