Merge lp:~gmb/maas-test/fix-up-proxy-config-dir-for-tests into lp:maas-test

[Graham Binns]

This branch makes the config_dir of LocalProxyFixture, and the ssh_key_dir of KVMFixture configurable. It also adds a command-line option, --config-dir, which sets both of the above (there's an argument for having ssh_key_dir separate, I suppose; I'm happy to do that if it's a better fit).

[lp:~gmb/maas-test/fix-up-proxy-config-dir-for-tests]

77. By Graham Binns on 2013-11-27

Fixed a typo.

78. By Graham Binns on 2013-11-27

Removed TODO that was TODONE.

[Gavin Panella (allenap)]

Looks good, but I'm concerned about punting everything to the user.

[1]

+    # maas-test config
+    parser.add_argument(
+        '--config-dir', type=text_type,
+        default="~/.maas-test",
+        help="The directory in which to store maas-test config and cache "
+        "files. Defaults to %(default)s")

I think we should use the xdg stuff I mentioned to rvb yesterday, and
not have yet another configuration option. Or is there a compelling
use-case for allowing this? This feels like getting the user to choose
because we can't make a decision.

review: Needs Information

[Julian Edwards (julian-edwards)]

On Wednesday 27 Nov 2013 16:28:29 you wrote:
> Review: Needs Information
>
> Looks good, but I'm concerned about punting everything to the user.
>
>
> [1]
>
> + # maas-test config
> + parser.add_argument(
> + '--config-dir', type=text_type,
> + default="~/.maas-test",
> + help="The directory in which to store maas-test config and cache "
> + "files. Defaults to %(default)s")
>
> I think we should use the xdg stuff I mentioned to rvb yesterday, and
> not have yet another configuration option. Or is there a compelling
> use-case for allowing this? This feels like getting the user to choose
> because we can't make a decision.

+1 to removing config where sane and and possible.

[Graham Binns (gmb)]

On 27 November 2013 22:51, Julian Edwards <email address hidden> wrote:
>> I think we should use the xdg stuff I mentioned to rvb yesterday, and
>> not have yet another configuration option. Or is there a compelling
>> use-case for allowing this? This feels like getting the user to choose
>> because we can't make a decision.

I saw that whizz past in a merge proposal and promptly forgot about
it. That makes perfect sense to me.

[Julian Edwards (julian-edwards)]

On Thursday 28 Nov 2013 07:21:34 you wrote:
> On 27 November 2013 22:51, Julian Edwards <email address hidden>
wrote:
> >> I think we should use the xdg stuff I mentioned to rvb yesterday, and
> >> not have yet another configuration option. Or is there a compelling
> >> use-case for allowing this? This feels like getting the user to choose
> >> because we can't make a decision.
>
> I saw that whizz past in a merge proposal and promptly forgot about
> it. That makes perfect sense to me.

For the record, I didn't say that, Gavin did :)

[Graham Binns (gmb)]

On 28 November 2013 07:24, Julian Edwards <email address hidden> wrote:
> For the record, I didn't say that, Gavin did :)

Yeah, trimming fail.

[lp:~gmb/maas-test/fix-up-proxy-config-dir-for-tests]

79. By Graham Binns on 2013-11-28

Removed config-dir option stuff; we'll use xdg instead.

80. By Graham Binns on 2013-11-28

XDG conversion.

81. By Graham Binns on 2013-11-28

Merged trunk.

82. By Graham Binns on 2013-11-28

Added dependencies.

83. By Graham Binns on 2013-11-28

Trimmed some lint.

84. By Graham Binns on 2013-11-28

Removed a TODO.

[Gavin Panella (allenap)]

Tip top. One more small thing:

[2]

-            os.path.expanduser('~'), '.maas-test', 'vm_ssh_id_rsa')
+            BaseDirectory.save_config_path('.maas-test'), 'vm_ssh_id_rsa')

No need for the leading . on .maas-test.

This appears elsewhere too.

Also, fwiw, any number of components can passed to save_config_path(),
for example:

            BaseDirectory.save_config_path('maas-test', 'vm-ssh')

would create a directory at $XDG_CONFIG_HOME/maas-test/vm-ssh.

review: Approve

[Graham Binns (gmb)]

On 28 November 2013 10:03, Gavin Panella <email address hidden> wrote:
> Review: Approve
>
> Tip top. One more small thing:
>
>
> [2]
>
> - os.path.expanduser('~'), '.maas-test', 'vm_ssh_id_rsa')
> + BaseDirectory.save_config_path('.maas-test'), 'vm_ssh_id_rsa')
>
> No need for the leading . on .maas-test.
>
> This appears elsewhere too.

On it.

> Also, fwiw, any number of components can passed to save_config_path(),
> for example:
>
> BaseDirectory.save_config_path('maas-test', 'vm-ssh')
>
> would create a directory at $XDG_CONFIG_HOME/maas-test/vm-ssh.

Ah, but we don't want to create a directory there (I've improved the
code so that's more obvious). vm_ssh_id_rsa is the name of the
keyfile, so we need to make sure the directory is there and then
create the path to the file from it. As I said, I've improved the
clarity.

[lp:~gmb/maas-test/fix-up-proxy-config-dir-for-tests]

85. By Graham Binns on 2013-11-28

Fixed an assortment of wonderful things.

86. By Graham Binns on 2013-11-28

Removed unused imports.

[Gavin Panella (allenap)]

> Ah, but we don't want to create a directory there (I've improved the
> code so that's more obvious).

I did understand that - your code was clear - I was just pointing it out
for future reference.

[Graham Binns (gmb)]

On 28 November 2013 14:25, Gavin Panella <email address hidden> wrote:
> I did understand that - your code was clear - I was just pointing it out
> for future reference.

Okay, cool. Thanks for the clarification.

=== modified file 'maastest/cases.py'
--- maastest/cases.py	2013-11-27 17:22:21 +0000
+++ maastest/cases.py	2013-11-28 14:16:14 +0000
@@ -129,11 +129,12 @@
             "URL: http://%s/MAAS/\n"
             "Username: %s\n"
             "Password: %s\n"
-            "SSH login: ssh -i ~/.maas-test/vm_ssh_id_rsa %s\n"
+            "SSH login: ssh -i %s %s\n"
             % (
                 self.machine.ip_address(),
                 self.maas.admin_user,
                 self.maas.admin_password,
+                self.machine.ssh_private_key_file,
                 self.machine.identity(),
             ))
 

=== modified file 'maastest/kvmfixture.py'
--- maastest/kvmfixture.py	2013-11-27 13:36:27 +0000
+++ maastest/kvmfixture.py	2013-11-28 14:16:14 +0000
@@ -34,6 +34,7 @@
 import netaddr
 from six import text_type
 from testtools.content import text_content
+from xdg import BaseDirectory
 
 # Maximum wait for the virtual machine to complete its cloud-init
 # initialization.  It can take quite a while, because it performs an
@@ -67,7 +68,7 @@
     """
     def __init__(self, series, architecture, proxy_url,
                  direct_interface=None, direct_network=None, name=None,
-                 archive=None):
+                 archive=None, ssh_key_dir=None):
         """Initialise a KVM-based Virtual Machine fixture.
 
         The VM's network is configured like this:
@@ -103,6 +104,10 @@
         :param archive: An optional repository name.  If provided, the
             repository will be added onto the VM.
         :type archive: string
+        :param ssh_key_dir: The directory in which to store the SSH keys
+            for connecting to this KVM instance. If None, this defaults
+            to the default XDG config directory.
+        :type ssh_key_dir: string.
         """
         super(KVMFixture, self).__init__()
         self.command_count = itertools.count(1)
@@ -124,8 +129,10 @@
         if name is None:
             name = text_type(uuid1())
         self.name = name
+        if ssh_key_dir is None:
+            ssh_key_dir = BaseDirectory.save_config_path('maas-test')
         self.ssh_private_key_file = os.path.join(
-            os.path.expanduser('~'), '.maas-test', 'vm_ssh_id_rsa')
+            ssh_key_dir, 'vm_ssh_id_rsa')
         self.running = False
 
     def setUp(self):

=== modified file 'maastest/proxyfixture.py'
--- maastest/proxyfixture.py	2013-11-27 13:36:27 +0000
+++ maastest/proxyfixture.py	2013-11-28 14:16:14 +0000
@@ -23,10 +23,9 @@
 from maastest import utils
 from netaddr import IPNetwork
 import netifaces
+from xdg import BaseDirectory
 
-# TODO: '.maas-test' shouln't be harcoded like that, it should
-# be a parameter passed to LocalProxyFixture().
-DISK_CACHE_ROOT = '~/.maas-test/proxy_cache/'
+DISK_CACHE_ROOT = '%(config_dir)s/proxy_cache/'
 
 PROXY_CONFIG_ITEMS = {
     'daemonise': 'true',
@@ -59,7 +58,7 @@
 
     #TODO: port should be determined at runtime from the set of
     # available ports rather than being an arbitrary one as it is here.
-    def __init__(self, port=42676):
+    def __init__(self, config_dir=None, port=42676):
         """Create a `LocalProxyFixture` object.
 
         :param url: The URL of the proxy to use. If None, LocalProxyFixture
@@ -68,13 +67,15 @@
         :param port: The port on which to start a local caching proxy.
             If URL is specified, this parameter is ignored.
         :type port: int
+        :param config_dir: The directory in which to put the proxy's
+            config, pid file and cache.
+        :type config_dir: string
         """
         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')
+        if config_dir is None:
+            config_dir = BaseDirectory.save_config_path('maas-test')
+        self.config_dir = config_dir
         self.pid_file = os.path.join(self.config_dir, 'proxy.pid')
         self._network_address = None
 
@@ -132,7 +133,7 @@
         logging.info("Done starting proxy.")
 
     def create_cache_root(self):
-        cache_root = os.path.expanduser(DISK_CACHE_ROOT)
+        cache_root = DISK_CACHE_ROOT % {'config_dir': self.config_dir}
         try:
             os.makedirs(cache_root)
         except OSError as error:

=== modified file 'maastest/tests/test_kvmfixture.py'
--- maastest/tests/test_kvmfixture.py	2013-11-27 17:44:58 +0000
+++ maastest/tests/test_kvmfixture.py	2013-11-28 14:16:14 +0000
@@ -13,8 +13,6 @@
 __all__ = []
 
 import itertools
-import os
-import os.path
 import random
 from random import randint
 import textwrap
@@ -49,9 +47,7 @@
         # KVMFixture.generate_ssh_key() generates an ssh key in ~/.maas-test,
         # creating that directory if necessary.  Set up a fake home directory
         # so it can do what it needs to.
-        home_dir = self.useFixture(fixtures.TempDir()).path
-        self.patch(
-            os.path, 'expanduser', mock.MagicMock(return_value=home_dir))
+        self.home_dir = self.useFixture(fixtures.TempDir()).path
 
     def patch_run_command(self, return_value=0, stdout=b'', stderr=b''):
         """Patch out `kvmfixture` calls to `run_command`.
@@ -84,7 +80,7 @@
             series, architecture, name=name,
             direct_interface=direct_interface,
             direct_network=direct_network, archive=archive,
-            proxy_url=proxy_url)
+            proxy_url=proxy_url, ssh_key_dir=self.home_dir)
         # Hack to get fixture.addDetail to work without actually calling
         # fixture.setUp().
         fixture._details = {}
@@ -288,7 +284,6 @@
             self.logger.output.strip().split("\n"))
 
     def test_generate_ssh_key_sets_up_temporary_key_pair(self):
-        home_dir = os.path.expanduser('~')
         fixture = self.make_KVMFixture()
         fixtures.Fixture.setUp(fixture)
 
@@ -297,7 +292,7 @@
         self.assertThat(fixture.ssh_private_key_file, FileExists())
         self.assertThat(fixture.get_ssh_public_key_file(), FileExists())
         self.assertThat(
-            fixture.ssh_private_key_file, StartsWith(home_dir + '/'))
+            fixture.ssh_private_key_file, StartsWith(self.home_dir + '/'))
 
         self.assertThat(
             fixture.ssh_private_key_file,

=== modified file 'maastest/tests/test_proxyfixture.py'
--- maastest/tests/test_proxyfixture.py	2013-11-26 22:51:09 +0000
+++ maastest/tests/test_proxyfixture.py	2013-11-28 14:16:14 +0000
@@ -39,24 +39,23 @@
                 'netmask': '255.255.255.0',
             }]}
         self.patch(netifaces, 'ifaddresses', mock_ifaddresses)
+        self.temp_dir = self.useFixture(TempDir()).path
 
     def get_random_port_number(self):
         return random.randint(1, 65535)
 
     def test_init_sets_properties(self):
         port = self.get_random_port_number()
-        proxy_fixture = LocalProxyFixture(port=port)
-        expected_config_path = os.path.join(
-            os.path.expanduser('~'), '.maas-test')
+        proxy_fixture = LocalProxyFixture(port=port, config_dir=self.temp_dir)
         self.assertEqual(port, proxy_fixture.port)
-        self.assertEqual(expected_config_path, proxy_fixture.config_dir)
+        self.assertEqual(self.temp_dir, proxy_fixture.config_dir)
         self.assertEqual(
-            os.path.join(expected_config_path, 'proxy.pid'),
+            os.path.join(self.temp_dir, 'proxy.pid'),
             proxy_fixture.pid_file)
 
     def test_get_url_returns_proxy_url(self):
         port = self.get_random_port_number()
-        proxy_fixture = LocalProxyFixture(port=port)
+        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir, port=port)
         self.assertEqual(
             "http://192.168.0.1:%s" % port,
             proxy_fixture.get_url())
@@ -65,8 +64,9 @@
         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:
+        port = self.get_random_port_number()
+        config_dir = self.temp_dir
+        with LocalProxyFixture(config_dir=config_dir, port=port) as fixture:
             pass
 
         self.assertEqual(
@@ -79,7 +79,8 @@
     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 = LocalProxyFixture(
+            config_dir=self.temp_dir, port=self.get_random_port_number())
 
         proxy_fixture.start_proxy()
 
@@ -90,26 +91,27 @@
             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 = LocalProxyFixture(
+            config_dir=self.temp_dir, port=self.get_random_port_number())
 
         proxy_fixture.create_cache_root()
-
-        self.assertThat(test_cache_path, DirExists())
+        expected_cache_path = proxyfixture.DISK_CACHE_ROOT % {
+            'config_dir': self.temp_dir}
+        self.assertThat(expected_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 = LocalProxyFixture(
+            config_dir=self.temp_dir, 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_fixture = LocalProxyFixture(
+            config_dir=self.temp_dir, port=self.get_random_port_number())
         proxy_adddress = proxy_fixture._get_network_address()
         self.assertEqual('192.168.0.1', proxy_adddress.ip.format())
         self.assertEqual(24, proxy_adddress.prefixlen)
@@ -120,7 +122,7 @@
         mock_read_file = mock.MagicMock()
         mock_read_file.return_value = bytes(self.getUniqueInteger())
         self.patch(utils, 'read_file', mock_read_file)
-        proxy_fixture = LocalProxyFixture()
+        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
         proxy_fixture.kill_running_proxy()
         self.assertEqual(
             mock.call(['kill', mock_read_file.return_value], check_call=True),
@@ -132,7 +134,7 @@
         mock_read_file = mock.MagicMock()
         mock_read_file.side_effect = Exception(self.getUniqueString())
         self.patch(utils, 'read_file', mock_read_file)
-        proxy_fixture = LocalProxyFixture()
+        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
         proxy_fixture.kill_running_proxy()
         mock_log_error.assert_has_calls([
             mock.call("Unable to kill proxy:"),
@@ -147,7 +149,7 @@
         mock_read_file = mock.MagicMock()
         mock_read_file.return_value = bytes(self.getUniqueInteger())
         self.patch(utils, 'read_file', mock_read_file)
-        proxy_fixture = LocalProxyFixture()
+        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
         proxy_fixture.kill_running_proxy()
         mock_log_error.assert_has_calls([
             mock.call("Unable to kill proxy:"),
@@ -162,7 +164,7 @@
         mock_kill_running_proxy = mock.MagicMock()
 
         port = self.get_random_port_number()
-        proxy_fixture = LocalProxyFixture(port)
+        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir, port=port)
         self.patch(
             proxy_fixture, 'kill_running_proxy', mock_kill_running_proxy)
 
@@ -174,5 +176,5 @@
         mock_exists = mock.MagicMock()
         mock_exists.return_value = True
         self.patch(os.path, 'exists', mock_exists)
-        proxy_fixture = LocalProxyFixture()
+        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
         self.assertRaises(Exception, proxy_fixture.setUp)

=== modified file 'packages.txt'
--- packages.txt	2013-11-26 13:10:43 +0000
+++ packages.txt	2013-11-28 14:16:14 +0000
@@ -11,6 +11,7 @@
 python-testresources
 python-testtools
 python-virtualenv
+python-xdg
 python3-fixtures
 python3-lxml
 python3-mock
@@ -18,6 +19,7 @@
 python3-six
 python3-testresources
 python3-testtools
+python3-xdg
 qemu-kvm
 uvtool
 uvtool-libvirt