DynamicInventory filesystem storage API cleanup

Update some function docstrings for accuracy/completeness
Make some methods module-private as they are not required to be
available for external use.

Tests were updated to accomodate for the now private functions.
The affected tests were identified as filesystem-centric and
moved to their own testing file, which will run under the inventory
tox target as well.

Parent-Id: I4d52f7363e00bba62ed878fe8fa6e593eb21fdde
Change-Id: I0275eeeafeb2a4c89cd820d56dd0ad01d3124a80

lib/filesystem.py

@@ -29,6 +29,13 @@ INVENTORY_FILENAME = 'openstack_inventory.json'
 
 
 def _get_search_paths(preferred_path=None, suffix=None):
+    """Return a list of search paths, including the standard location
+
+    :param preferred_path: A search path to prefer to a standard location
+    :param suffix: Appended to the search paths, e.g. subdirectory or filename
+    :return: ``(list)`` Path strings to search
+    """
+
     search_paths = [
         os.path.join(
             '/etc', 'openstack_deploy'
@@ -48,9 +55,8 @@ def file_find(filename, preferred_path=None, pass_exception=False):
 
     If no file is found and pass_exception is True, the system will exit.
     The file lookup will be done in the following directories:
-      ``preferred_path`` [Optional]
-      /etc/openstack_deploy/
-      $(pwd)/openstack_deploy/
+      * ``preferred_path`` [Optional]
+      * ``/etc/openstack_deploy/``
 
     :param filename: ``str``  Name of the file to find
     :param preferred_path: ``str`` Additional directory to look in FIRST
@@ -70,27 +76,39 @@ def file_find(filename, preferred_path=None, pass_exception=False):
             return False
 
 
-def make_backup(config_path, inventory_file_path):
-    # Create a backup of all previous inventory files as a tar archive
+def _make_backup(backup_path, source_file_path):
+    """ Create a backup of all previous inventory files as a tar archive
+
+    :param backup_path: where to store the backup file
+    :param source_file_path: path of file to backup
+    :return:
+    """
+
     inventory_backup_file = os.path.join(
-        config_path,
+        backup_path,
         'backup_openstack_inventory.tar'
     )
     with tarfile.open(inventory_backup_file, 'a') as tar:
-        basename = os.path.basename(inventory_file_path)
-        backup_name = get_backup_name(basename)
-        tar.add(inventory_file_path, arcname=backup_name)
+        basename = os.path.basename(source_file_path)
+        backup_name = _get_backup_name(basename)
+        tar.add(source_file_path, arcname=backup_name)
     logger.debug("Backup written to {}".format(inventory_backup_file))
 
 
-def get_backup_name(basename):
+def _get_backup_name(basename):
+    """ Return a name for a backup file based on the time
+
+    :param basename: serves as prefix for the return value
+    :return: a name for a backup file based on current time
+    """
+
     utctime = datetime.datetime.utcnow()
     utctime = utctime.strftime("%Y%m%d_%H%M%S")
     return '{}-{}.json'.format(basename, utctime)
 
 
 def load_from_json(filename, preferred_path=None, pass_exception=False):
-    """Return a dictionary found in a given file
+    """Return a dictionary found in json format in a given file
 
     :param filename: ``str``  Name of the file to read from
     :param preferred_path: ``str``  Path to the json file to try FIRST
@@ -124,7 +142,7 @@ def load_inventory(preferred_path=None, default_inv=None):
                                             pass_exception=True)
     if inventory is not False:
         logger.debug("Loaded existing inventory from {}".format(file_loaded))
-        make_backup(preferred_path, file_loaded)
+        _make_backup(preferred_path, file_loaded)
     else:
         logger.debug("No existing inventory, created fresh skeleton.")
         inventory = copy.deepcopy(default_inv)

tests/test_filesystem.py

@@ -0,0 +1,92 @@
+#!/usr/bin/env python
+
+import mock
+import os
+from os import path
+import sys
+import unittest
+
+from test_inventory import cleanup
+from test_inventory import get_inventory
+from test_inventory import make_config
+
+INV_DIR = 'playbooks/inventory'
+LIB_DIR = 'lib'
+
+sys.path.append(path.join(os.getcwd(), LIB_DIR))
+sys.path.append(path.join(os.getcwd(), INV_DIR))
+
+import filesystem as fs
+
+TARGET_DIR = path.join(os.getcwd(), 'tests', 'inventory')
+USER_CONFIG_FILE = path.join(TARGET_DIR, 'openstack_user_config.yml')
+
+
+def setUpModule():
+    # The setUpModule function is used by the unittest framework.
+    make_config()
+
+
+def tearDownModule():
+    # This file should only be removed after all tests are run,
+    # thus it is excluded from cleanup.
+    os.remove(USER_CONFIG_FILE)
+
+
+class TestMultipleRuns(unittest.TestCase):
+    def test_creating_backup_file(self):
+        inventory_file_path = os.path.join(TARGET_DIR,
+                                           'openstack_inventory.json')
+        get_backup_name_path = 'filesystem._get_backup_name'
+        backup_name = 'openstack_inventory.json-20160531_171804.json'
+
+        tar_file = mock.MagicMock()
+        tar_file.__enter__.return_value = tar_file
+
+        # run make backup with faked tarfiles and date
+        with mock.patch('filesystem.tarfile.open') as tar_open:
+            tar_open.return_value = tar_file
+            with mock.patch(get_backup_name_path) as backup_mock:
+                backup_mock.return_value = backup_name
+                fs._make_backup(TARGET_DIR, inventory_file_path)
+
+        backup_path = path.join(TARGET_DIR, 'backup_openstack_inventory.tar')
+
+        tar_open.assert_called_with(backup_path, 'a')
+
+        # This chain is present because of how tarfile.open is called to
+        # make a context manager inside the make_backup function.
+
+        tar_file.add.assert_called_with(inventory_file_path,
+                                        arcname=backup_name)
+
+    def test_recreating_files(self):
+        # Deleting the files after the first run should cause the files to be
+        # completely remade
+        get_inventory()
+
+        get_inventory()
+
+        backup_path = path.join(TARGET_DIR, 'backup_openstack_inventory.tar')
+
+        self.assertFalse(os.path.exists(backup_path))
+
+    def test_rereading_files(self):
+        # Generate the initial inventory files
+        get_inventory(clean=False)
+
+        inv = fs.load_inventory(TARGET_DIR)
+        self.assertIsInstance(inv, dict)
+        self.assertIn('_meta', inv)
+        # This test is basically just making sure we get more than
+        # INVENTORY_SKEL populated, so we're not going to do deep testing
+        self.assertIn('log_hosts', inv)
+
+    def tearDown(self):
+        # Clean up here since get_inventory will not do it by design in
+        # this test.
+        cleanup()
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/test_inventory.py

@@ -841,61 +841,6 @@ class TestGlobalOverridesConfigDeletion(TestConfigCheckBase):
         self.assertEqual('bar', self.inventory['all']['vars']['foo'])
 
 
-class TestMultipleRuns(unittest.TestCase):
-    def test_creating_backup_file(self):
-        inventory_file_path = os.path.join(TARGET_DIR,
-                                           'openstack_inventory.json')
-        get_backup_name_path = 'filesystem.get_backup_name'
-        backup_name = 'openstack_inventory.json-20160531_171804.json'
-
-        tar_file = mock.MagicMock()
-        tar_file.__enter__.return_value = tar_file
-
-        # run make backup with faked tarfiles and date
-        with mock.patch('filesystem.tarfile.open') as tar_open:
-            tar_open.return_value = tar_file
-            with mock.patch(get_backup_name_path) as backup_mock:
-                backup_mock.return_value = backup_name
-                fs.make_backup(TARGET_DIR, inventory_file_path)
-
-        backup_path = path.join(TARGET_DIR, 'backup_openstack_inventory.tar')
-
-        tar_open.assert_called_with(backup_path, 'a')
-
-        # This chain is present because of how tarfile.open is called to
-        # make a context manager inside the make_backup function.
-
-        tar_file.add.assert_called_with(inventory_file_path,
-                                        arcname=backup_name)
-
-    def test_recreating_files(self):
-        # Deleting the files after the first run should cause the files to be
-        # completely remade
-        get_inventory()
-
-        get_inventory()
-
-        backup_path = path.join(TARGET_DIR, 'backup_openstack_inventory.tar')
-
-        self.assertFalse(os.path.exists(backup_path))
-
-    def test_rereading_files(self):
-        # Generate the initial inventory files
-        get_inventory(clean=False)
-
-        inv = fs.load_inventory(TARGET_DIR)
-        self.assertIsInstance(inv, dict)
-        self.assertIn('_meta', inv)
-        # This test is basically just making sure we get more than
-        # INVENTORY_SKEL populated, so we're not going to do deep testing
-        self.assertIn('log_hosts', inv)
-
-    def tearDown(self):
-        # Clean up here since get_inventory will not do it by design in
-        # this test.
-        cleanup()
-
-
 class TestEnsureInventoryUptoDate(unittest.TestCase):
     def setUp(self):
         self.env = di.load_environment(BASE_ENV_DIR, {})

tox.ini

@@ -152,6 +152,7 @@ commands =
     coverage run -a {toxinidir}/tests/test_inventory.py
     coverage run -a {toxinidir}/tests/test_manage.py
     coverage run -a {toxinidir}/tests/test_ip.py
+    coverage run -a {toxinidir}/tests/test_filesystem.py
     coverage report --show-missing --include={toxinidir}/playbooks/inventory/*,{toxinidir}/lib/*