Merge lp:~milo/lava-tool/bug1206579 into lp:~linaro-validation/lava-tool/trunk

[Milo Casagrande]

Added initial support for XDG directories.

The Config and InteractiveConfig class now will default to use $XDG_CONFIG_HOME as the directory where to store configurations. The dir tree will be as follows: $XDG_CONFIG_HOME/linaro/lava-tool/. The filename used here is "lava-tool.ini".

The "linaro" part of the path, if somebody is using linaro-image-tools or other CLI tools, is already in place.

A new class, that inherits from InteractiveConfig has been introduced: InteractiveCache. This is exactly the same class as the other config ones, only it stores values in $XDG_CACHE_HOME/linaro/lava-tool/. The filename used here is "parameters.ini".

The BaseCommand class has been fixed to use the new InteractiveCache class.

Tests have been added and fixed where necessary.

[Antonio Terceiro (terceiro)]

Looks good to me. :-)

 review approve

> The BaseCommand class has been fixed to use the new InteractiveCache
> class.

So effectively InteractiveConfig in itself is now unused, apart from
being the base class for InteractiveCache. Config was already not used
directly ...

Maybe we could collapse the entire Config ← InteractiveConfig ←
InteractiveCache hierarchy into a single class?

Or do we want to create a scheme where the user *can* create an actual
config file that will provide defaults when prompting? Maybe a single
config object could load both config and cache files, and when something
is not found on cache, it falls back to the value in the config as
default when prompting ...

(not suggesting to do this for this MP though - just food for thought)

review: Approve

[Milo Casagrande (milo)]

On Thu, Aug 1, 2013 at 2:25 AM, Antonio Terceiro
<email address hidden> wrote:
> Review: Approve
>
> Looks good to me. :-)
>
> review approve
>
>> The BaseCommand class has been fixed to use the new InteractiveCache
>> class.
>
> So effectively InteractiveConfig in itself is now unused, apart from
> being the base class for InteractiveCache. Config was already not used
> directly ...
>
> Maybe we could collapse the entire Config ← InteractiveConfig ←
> InteractiveCache hierarchy into a single class?

I still value separating Config and InteractiveConfig, but I agree we
should find a better way for InteractiveCache.

> Or do we want to create a scheme where the user *can* create an actual
> config file that will provide defaults when prompting? Maybe a single
> config object could load both config and cache files, and when something
> is not found on cache, it falls back to the value in the config as
> default when prompting ...

Sounds as a reasonable trade-off to me.
If I have a little bit of time I can work on it. Will file a bug.

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

=== modified file 'integration-tests.d/lava-job-new-with-config.sh'
--- integration-tests.d/lava-job-new-with-config.sh	2013-05-28 22:08:12 +0000
+++ integration-tests.d/lava-job-new-with-config.sh	2013-07-31 12:49:45 +0000
@@ -5,6 +5,6 @@
 [device_type=panda]
 prebuilt_image = file:///path/to/panda.img
 EOF
-LAVACONFIG="${tmpdir}/config" lava job new "${tmpdir}/job.json"
+LAVACACHE="${tmpdir}/config" lava job new "${tmpdir}/job.json"
 cat "${tmpdir}/job.json"
 grep "device_type.*panda" "${tmpdir}/job.json" && grep "image.*path.to.panda.img" "${tmpdir}/job.json"

=== modified file 'lava/config.py'
--- lava/config.py	2013-07-26 13:48:06 +0000
+++ lava/config.py	2013-07-31 12:49:45 +0000
@@ -23,6 +23,7 @@
 import atexit
 import os
 import readline
+import xdg.BaseDirectory as xdgBaseDir
 
 from ConfigParser import (
     ConfigParser,
@@ -33,12 +34,16 @@
 from lava.parameter import Parameter
 from lava.tool.errors import CommandError
 
-__all__ = ['Config', 'InteractiveConfig']
+__all__ = ['Config', 'InteractiveCache', 'InteractiveConfig']
 
 # Store for function calls to be made at exit time.
 AT_EXIT_CALLS = set()
 # Config default section.
 DEFAULT_SECTION = "DEFAULT"
+# This is the default base name used to create XDG resources.
+DEFAULT_XDG_RESOURCE = "linaro"
+# This is the default name for lava-tool resources.
+DEFAULT_LAVA_TOOL_RESOURCE = "lava-tool"
 
 HISTORY = os.path.join(os.path.expanduser("~"), ".lava_history")
 try:
@@ -69,8 +74,8 @@
     def config_file(self):
         if self._config_file is None:
             self._config_file = (os.environ.get('LAVACONFIG') or
-                                 os.path.join(os.path.expanduser('~'),
-                                              '.lavaconfig'))
+                                 os.path.join(self._ensure_xdg_dirs(),
+                                              'lava-tool.ini'))
         return self._config_file
 
     @config_file.setter
@@ -84,6 +89,14 @@
             self._config_backend.read([self.config_file])
         return self._config_backend
 
+    def _ensure_xdg_dirs(self):
+        """Make sure we have the default resource.
+
+        :return The path to the XDG resource.
+        """
+        return xdgBaseDir.save_config_path(DEFAULT_XDG_RESOURCE,
+                                           DEFAULT_LAVA_TOOL_RESOURCE)
+
     def _calculate_config_section(self, parameter):
         """Calculates the config section of the specified parameter.
 
@@ -248,3 +261,34 @@
         if value is not None and parameter.store:
             self.put(parameter.id, value, section)
         return value
+
+
+class InteractiveCache(InteractiveConfig):
+
+    """An interactive cache where parameters that can change are stored.
+
+    This class is basically the same as the Confing and InteractiveConfig ones,
+    only the base directory where the cache file is stored is different.
+
+    In this case it will use the $XDG_CACHE_HOME value as defined in XDG.
+    """
+
+    @property
+    def config_file(self):
+        if self._config_file is None:
+            self._config_file = (os.environ.get('LAVACACHE') or
+                                 os.path.join(self._ensure_xdg_dirs(),
+                                              'parameters.ini'))
+        return self._config_file
+
+    @config_file.setter
+    def config_file(self, value):
+        self._config_file = value
+
+    def _ensure_xdg_dirs(self):
+        """Make sure we have the default resource.
+
+        :return The path to the XDG resource.
+        """
+        return xdgBaseDir.save_cache_path(DEFAULT_XDG_RESOURCE,
+                                          DEFAULT_LAVA_TOOL_RESOURCE)

=== modified file 'lava/helper/command.py'
--- lava/helper/command.py	2013-07-29 10:09:09 +0000
+++ lava/helper/command.py	2013-07-31 12:49:45 +0000
@@ -22,7 +22,9 @@
 import sys
 import xmlrpclib
 
-from lava.config import InteractiveConfig
+from lava.config import (
+    InteractiveCache,
+)
 from lava.helper.dispatcher import get_devices
 from lava.job import Job
 from lava.job.templates import (
@@ -58,7 +60,7 @@
     verify_and_create_url,
 )
 
-CONFIG = InteractiveConfig()
+CONFIG = InteractiveCache()
 
 
 class BaseCommand(Command):

=== modified file 'lava/testdef/tests/test_commands.py'
--- lava/testdef/tests/test_commands.py	2013-07-26 08:10:16 +0000
+++ lava/testdef/tests/test_commands.py	2013-07-31 12:49:45 +0000
@@ -25,10 +25,11 @@
 import yaml
 
 from mock import (
+    MagicMock,
     patch,
 )
 
-from lava.config import InteractiveConfig
+from lava.config import InteractiveCache
 from lava.helper.tests.helper_test import HelperTest
 from lava.testdef.commands import (
     new,
@@ -39,8 +40,7 @@
 class NewCommandTest(HelperTest):
     """Class for the lava.testdef new command tests."""
 
-    @patch("lava.config.Config.save")
-    def setUp(self, mocked_save):
+    def setUp(self):
         super(NewCommandTest, self).setUp()
         self.file_name = "fake_testdef.yaml"
         self.file_path = os.path.join(tempfile.gettempdir(), self.file_name)
@@ -50,7 +50,8 @@
                                                      delete=False)
 
         self.config_file = tempfile.NamedTemporaryFile(delete=False)
-        self.config = InteractiveConfig()
+        self.config = InteractiveCache()
+        self.config.save = MagicMock()
         self.config.config_file = self.config_file.name
         # Patch class raw_input, start it, and stop it on tearDown.
         self.patcher1 = patch("lava.parameter.raw_input", create=True)
@@ -84,6 +85,7 @@
         # file system.
         self.mocked_raw_input.return_value = "\n"
         new_command = new(self.parser, self.args)
+        new_command.config = self.config
         new_command.invoke()
         self.assertTrue(os.path.exists(self.file_path))
 
@@ -92,6 +94,7 @@
         # thrown.
         self.args.FILE = self.temp_yaml.name
         new_command = new(self.parser, self.args)
+        new_command.config = self.config
         self.assertRaises(CommandError, new_command.invoke)
 
     def test_invoke_2(self):
@@ -124,6 +127,7 @@
         self.args.FILE = "/test_file.yaml"
         self.mocked_raw_input.return_value = "\n"
         new_command = new(self.parser, self.args)
+        new_command.config = self.config
         self.assertRaises(CommandError, new_command.invoke)
         self.assertFalse(os.path.exists(self.args.FILE))
 
@@ -133,6 +137,7 @@
         self.mocked_raw_input.side_effect = ["foo", "\n", "\n", "\n", "\n",
                                              "\n"]
         new_command = new(self.parser, self.args)
+        new_command.config = self.config
         new_command.invoke()
         expected = {'run': {'steps': ["./mytest.sh"]},
                     'metadata': {

=== modified file 'lava/tests/test_config.py'
--- lava/tests/test_config.py	2013-07-26 08:10:16 +0000
+++ lava/tests/test_config.py	2013-07-31 12:49:45 +0000
@@ -20,6 +20,8 @@
 lava.config unit tests.
 """
 
+import os
+import shutil
 import sys
 
 from StringIO import StringIO
@@ -31,6 +33,7 @@
 
 from lava.config import (
     Config,
+    InteractiveCache,
     InteractiveConfig,
 )
 from lava.helper.tests.helper_test import HelperTest
@@ -49,17 +52,75 @@
         self.param2 = Parameter("bar", depends=self.param1)
 
 
+class TestConfigSave(ConfigTestCase):
+
+    """Used to test the save() method of config class.
+
+    Done here since in the other tests we want to mock the atexit save call
+    in order not to write the file, or accidentaly overwrite the real
+    user file.
+    """
+
+    def setUp(self):
+        super(TestConfigSave, self).setUp()
+        self.config = Config()
+        self.config.config_file = self.temp_file.name
+
+    def test_config_save(self):
+        self.config.put_parameter(self.param1, "foo")
+        self.config.save()
+
+        expected = "[DEFAULT]\nfoo = foo\n\n"
+        obtained = ""
+        with open(self.temp_file.name) as tmp_file:
+            obtained = tmp_file.read()
+        self.assertEqual(expected, obtained)
+
+    def test_save_list_param(self):
+        # Tests that when saved to file, the ListParameter parameter is stored
+        # correctly.
+        param_values = ["foo", "more than one words", "bar"]
+        list_param = ListParameter("list")
+        list_param.set(param_values)
+
+        self.config.put_parameter(list_param, param_values)
+        self.config.save()
+
+        expected = "[DEFAULT]\nlist = " + ",".join(param_values) + "\n\n"
+        obtained = ""
+        with open(self.temp_file.name, "r") as read_file:
+            obtained = read_file.read()
+        self.assertEqual(expected, obtained)
+
+
 class ConfigTest(ConfigTestCase):
 
-    @patch("lava.config.Config.save")
-    def setUp(self, mocked_save):
+    def setUp(self):
         super(ConfigTest, self).setUp()
+        self.patcher = patch("lava.config.DEFAULT_XDG_RESOURCE", "a_temp_dir")
+        self.patcher.start()
+        self.xdg_resource = os.path.join(
+            os.path.expanduser("~"), ".config/a_temp_dir")
+        self.lavatool_resource = os.path.join(self.xdg_resource, "lava-tool")
         self.config = Config()
-        self.config.config_file = self.temp_file.name
-
-    def test_assert_temp_config_file(self):
-        # Dummy test to make sure we are overriding correctly the Config class.
-        self.assertEqual(self.config.config_file, self.temp_file.name)
+        self.config.save = MagicMock()
+
+    def tearDown(self):
+        super(ConfigTest, self).tearDown()
+        self.patcher.stop()
+        if os.path.isdir(self.xdg_resource):
+            shutil.rmtree(self.xdg_resource)
+
+    def test_ensure_xdg_dirs(self):
+        # Test that xdg can create the correct cache path, we remove it
+        # at the end since we patch the default value.
+        obtained = self.config._ensure_xdg_dirs()
+        self.assertEquals(self.lavatool_resource, obtained)
+
+    def test_config_file(self):
+        expected = os.path.join(self.lavatool_resource, "lava-tool.ini")
+        obtained = self.config.config_file
+        self.assertEquals(expected, obtained)
 
     def test_config_put_in_cache_0(self):
         self.config._put_in_cache("key", "value", "section")
@@ -138,16 +199,6 @@
         obtained = self.config._calculate_config_section(self.param2)
         self.assertEqual(expected, obtained)
 
-    def test_config_save(self):
-        self.config.put_parameter(self.param1, "foo")
-        self.config.save()
-
-        expected = "[DEFAULT]\nfoo = foo\n\n"
-        obtained = ""
-        with open(self.temp_file.name) as tmp_file:
-            obtained = tmp_file.read()
-        self.assertEqual(expected, obtained)
-
     def test_config_get_from_backend_public(self):
         # Need to to this, since we want a clean Config instance, with
         # a config_file with some content.
@@ -160,10 +211,10 @@
 
 class InteractiveConfigTest(ConfigTestCase):
 
-    @patch("lava.config.Config.save")
-    def setUp(self, mocked_save):
+    def setUp(self):
         super(InteractiveConfigTest, self).setUp()
         self.config = InteractiveConfig()
+        self.config.save = MagicMock()
         self.config.config_file = self.temp_file.name
 
     @patch("lava.config.Config.get", new=MagicMock(return_value=None))
@@ -264,18 +315,38 @@
         self.assertIsInstance(obtained, list)
         self.assertEqual(expected, obtained)
 
-    def test_interactive_save_list_param(self):
-        # Tests that when saved to file, the ListParameter parameter is stored
-        # correctly.
-        param_values = ["foo", "more than one words", "bar"]
-        list_param = ListParameter("list")
-        list_param.set(param_values)
-
-        self.config.put_parameter(list_param, param_values)
-        self.config.save()
-
-        expected = "[DEFAULT]\nlist = " + ",".join(param_values) + "\n\n"
-        obtained = ""
-        with open(self.temp_file.name, "r") as read_file:
-            obtained = read_file.read()
-        self.assertEqual(expected, obtained)
+
+class TestInteractiveCache(HelperTest):
+
+    def setUp(self):
+        super(TestInteractiveCache, self).setUp()
+        self.patcher = patch("lava.config.DEFAULT_XDG_RESOURCE", "a_temp_dir")
+        self.patcher.start()
+        self.cache = InteractiveCache()
+        self.cache.save = MagicMock()
+        self.xdg_resource = os.path.join(
+            os.path.expanduser("~"), ".cache/a_temp_dir")
+        self.lavatool_resource = os.path.join(self.xdg_resource, "lava-tool")
+
+    def tearDown(self):
+        super(TestInteractiveCache, self).tearDown()
+        self.patcher.stop()
+        if os.path.isdir(self.xdg_resource):
+            shutil.rmtree(self.xdg_resource)
+
+    def test_default_xdg(self):
+        # Dummy test, only to make sure patching the module attribute works.
+        from lava.config import DEFAULT_XDG_RESOURCE
+        self.assertEquals(DEFAULT_XDG_RESOURCE, "a_temp_dir")
+
+    def test_ensure_xdg_dirs(self):
+        # Test that xdg can create the correct cache path, we remove it
+        # at the end since we patch the default value.
+        obtained = self.cache._ensure_xdg_dirs()
+        self.assertEquals(self.lavatool_resource, obtained)
+
+    def test_config_file(self):
+        expected = os.path.join(self.lavatool_resource, "parameters.ini")
+        obtained = self.cache.config_file
+        self.assertEquals(expected, obtained)
+

=== modified file 'setup.py'
--- setup.py	2013-07-17 14:30:42 +0000
+++ setup.py	2013-07-31 12:49:45 +0000
@@ -51,7 +51,8 @@
         'argcomplete >= 0.3',
         'keyring',
         'json-schema-validator >= 2.0',
-        'versiontools >= 1.3.1'
+        'versiontools >= 1.3.1',
+        'pyxdg == 0.25',
     ],
     setup_requires=['versiontools >= 1.3.1'],
     tests_require=[