Fixing the recursive path handling.

The patch f2830c5244 introduced
several potentially breaking bugs, which have to be fixed.
Namely, several of the termination conditionals lacked proper
return statements.

In addition to the fix of the previously describe issues
the following changes are included to prevent more arising in
the future.

Every major behavioral branch now has a dedicated log statement.
Test coverage included.
Expanded docstrings.
Increased expressiveness of var names.

Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Change-Id: Ie7471361acf1f4ae197e76b22eb7ba4d3f62fe92

validations_libs/tests/fakes.py

@@ -13,6 +13,8 @@
 #   under the License.
 #
 
+from validations_libs import constants
+
 VALIDATIONS_LIST = [{
     'description': 'My Validation One Description',
     'groups': ['prep', 'pre-deployment'],
@@ -300,3 +302,9 @@ FAKE_VALIDATIONS_PATH = '/usr/share/ansible/validation-playbooks'
 
 def fake_ansible_runner_run_return(status='successful', rc=0):
     return status, rc
+
+
+def _accept_default_log_path(path, *args):
+    if path == constants.VALIDATIONS_LOG_BASEDIR:
+        return True
+    return False

validations_libs/tests/test_utils.py

@@ -17,9 +17,10 @@ try:
     from unittest import mock
 except ImportError:
     import mock
+
 from unittest import TestCase
 
-from validations_libs import utils
+from validations_libs import utils, constants
 from validations_libs.tests import fakes
 
 
@@ -211,3 +212,99 @@ class TestUtils(TestCase):
         self.assertRaises(TypeError,
                           utils.convert_data,
                           data=data_dict)
+
+    @mock.patch('validations_libs.utils.LOG', autospec=True)
+    @mock.patch('validations_libs.utils.os.makedirs')
+    @mock.patch(
+        'validations_libs.utils.os.access',
+        side_effect=[False, True])
+    @mock.patch('validations_libs.utils.os.path.exists', return_value=True)
+    def test_create_log_dir_access_issue(self, mock_exists,
+                                         mock_access, mock_mkdirs,
+                                         mock_log):
+        log_path = utils.create_log_dir("/foo/bar")
+        self.assertEqual(log_path, constants.VALIDATIONS_LOG_BASEDIR)
+
+    @mock.patch('validations_libs.utils.LOG', autospec=True)
+    @mock.patch(
+        'validations_libs.utils.os.makedirs',
+        side_effect=PermissionError)
+    @mock.patch(
+        'validations_libs.utils.os.access',
+        autospec=True,
+        return_value=True)
+    @mock.patch(
+        'validations_libs.utils.os.path.exists',
+        autospec=True,
+        side_effect=fakes._accept_default_log_path)
+    def test_create_log_dir_existence_issue(self, mock_exists,
+                                            mock_access, mock_mkdirs,
+                                            mock_log):
+        """Tests behavior after encountering non-existence
+        of the the selected log folder, failed attempt to create it
+        (raising PermissionError), and finally resorting to a fallback.
+        """
+        log_path = utils.create_log_dir("/foo/bar")
+        self.assertEqual(log_path, constants.VALIDATIONS_LOG_BASEDIR)
+
+    @mock.patch('validations_libs.utils.LOG', autospec=True)
+    @mock.patch('validations_libs.utils.os.makedirs')
+    @mock.patch('validations_libs.utils.os.access', return_value=True)
+    @mock.patch('validations_libs.utils.os.path.exists', return_value=True)
+    def test_create_log_dir_success(self, mock_exists,
+                                    mock_access, mock_mkdirs,
+                                    mock_log):
+        """Test successful log dir retrieval on the first try.
+        """
+        log_path = utils.create_log_dir("/foo/bar")
+        self.assertEqual(log_path, "/foo/bar")
+
+    @mock.patch('validations_libs.utils.LOG', autospec=True)
+    @mock.patch(
+        'validations_libs.utils.os.makedirs',
+        side_effect=PermissionError)
+    @mock.patch('validations_libs.utils.os.access', return_value=False)
+    @mock.patch('validations_libs.utils.os.path.exists', return_value=False)
+    def test_create_log_dir_runtime_err(self, mock_exists,
+                                        mock_access, mock_mkdirs,
+                                        mock_log):
+        """Test if failure of the fallback raises 'RuntimeError'
+        """
+        self.assertRaises(RuntimeError, utils.create_log_dir, "/foo/bar")
+
+    @mock.patch('validations_libs.utils.LOG', autospec=True)
+    @mock.patch(
+        'validations_libs.utils.os.makedirs',
+        side_effect=PermissionError)
+    @mock.patch('validations_libs.utils.os.access', return_value=False)
+    @mock.patch(
+        'validations_libs.utils.os.path.exists',
+        side_effect=fakes._accept_default_log_path)
+    def test_create_log_dir_default_perms_runtime_err(
+                                        self, mock_exists,
+                                        mock_access, mock_mkdirs,
+                                        mock_log):
+        """Test if the inaccessible fallback raises 'RuntimeError'
+        """
+        self.assertRaises(RuntimeError, utils.create_log_dir, "/foo/bar")
+
+    @mock.patch('validations_libs.utils.LOG', autospec=True)
+    @mock.patch('validations_libs.utils.os.makedirs')
+    @mock.patch('validations_libs.utils.os.access', return_value=False)
+    @mock.patch('validations_libs.utils.os.path.exists', return_value=False)
+    def test_create_log_dir_mkdirs(self, mock_exists,
+                                   mock_access, mock_mkdirs,
+                                   mock_log):
+        """Test successful creation of the directory if the first access fails.
+        """
+
+        log_path = utils.create_log_dir("/foo/bar")
+        self.assertEqual(log_path, "/foo/bar")
+
+    @mock.patch(
+        'validations_libs.utils.os.makedirs',
+        side_effect=PermissionError)
+    def test_create_artifacts_dir_runtime_err(self, mock_mkdirs):
+        """Test if failure to create artifacts dir raises 'RuntimeError'.
+        """
+        self.assertRaises(RuntimeError, utils.create_artifacts_dir, "/foo/bar")

validations_libs/tests/test_validation_actions.py

@@ -45,9 +45,11 @@ class TestValidationActions(TestCase):
                                               'My Validation Two Name',
                                              ['prep', 'pre-introspection'])]))
 
+    @mock.patch('validations_libs.utils.os.access', return_value=True)
+    @mock.patch('validations_libs.utils.os.path.exists', return_value=True)
     @mock.patch('validations_libs.utils.get_validations_playbook',
                 return_value=['/tmp/foo/fake.yaml'])
-    def test_validation_skip_validation(self, mock_validation_play):
+    def test_validation_skip_validation(self, mock_validation_play, mock_exists, mock_access):
 
         playbook = ['fake.yaml']
         inventory = 'tmp/inventory.yaml'
@@ -184,13 +186,17 @@ class TestValidationActions(TestCase):
 
         mock_ansible_run.assert_called_with(**run_called_args)
 
+    @mock.patch('validations_libs.utils.os.makedirs')
+    @mock.patch('validations_libs.utils.os.access', return_value=True)
+    @mock.patch('validations_libs.utils.os.path.exists', return_value=True)
     @mock.patch('validations_libs.validation_actions.ValidationLogs.get_results',
                 side_effect=fakes.FAKE_SUCCESS_RUN)
     @mock.patch('validations_libs.utils.parse_all_validations_on_disk')
     @mock.patch('validations_libs.ansible.Ansible.run')
     def test_validation_run_success(self, mock_ansible_run,
                                     mock_validation_dir,
-                                    mock_results):
+                                    mock_results, mock_exists, mock_access,
+                                    mock_makedirs):
 
         mock_validation_dir.return_value = [{
             'description': 'My Validation One Description',
@@ -222,11 +228,16 @@ class TestValidationActions(TestCase):
                           validations_dir='/tmp/foo'
                           )
 
+    @mock.patch('validations_libs.utils.os.makedirs')
+    @mock.patch('validations_libs.utils.os.access', return_value=True)
+    @mock.patch('validations_libs.utils.os.path.exists', return_value=True)
     @mock.patch('validations_libs.validation_logs.ValidationLogs.get_results')
     @mock.patch('validations_libs.utils.parse_all_validations_on_disk')
     @mock.patch('validations_libs.ansible.Ansible.run')
     def test_validation_run_failed(self, mock_ansible_run,
-                                   mock_validation_dir, mock_results):
+                                   mock_validation_dir, mock_results,
+                                   mock_exists, mock_access,
+                                   mock_makedirs):
 
         mock_validation_dir.return_value = [{
             'description': 'My Validation One Description',

validations_libs/utils.py

@@ -33,56 +33,97 @@ def current_time():
 
 
 def create_log_dir(log_path=constants.VALIDATIONS_LOG_BASEDIR):
-    """Create default validations log dir.
+    """Check for presence of the selected validations log dir.
+    Create the directory if needed, and use fallback if that
+    proves too tall an order.
+
     Log the failure if encountering OSError or PermissionError.
-    :raises: RuntimeError
+
+    :param log_path: path of the selected log directory
+    :type log_path: `string`
+    :return: valid path to the log directory
+    :rtype: `string`
+
+    :raises: RuntimeError if even the fallback proves unavailable.
     """
     try:
         if os.path.exists(log_path):
             if os.access(log_path, os.W_OK):
                 return log_path
             else:
-                create_log_dir(constants.VALIDATIONS_LOG_BASEDIR)
+                LOG.error(
+                    (
+                        "Selected log directory '{log_path}' is inaccessible. "
+                        "Please check the access rights for: '{log_path}'"
+                    ).format(
+                        log_path=log_path))
+                if log_path != constants.VALIDATIONS_LOG_BASEDIR:
+                    LOG.warning(
+                    (
+                        "Resorting to the preset '{default_log_path}'"
+                    ).format(
+                        default_log_path=constants.VALIDATIONS_LOG_BASEDIR))
+
+                    return create_log_dir()
+                else:
+                    raise RuntimeError()
         else:
+            LOG.warning(
+                (
+                    "Selected log directory '{log_path}' does not exist. "
+                    "Attempting to create it."
+                ).format(
+                    log_path=log_path))
+
             os.makedirs(log_path)
             return log_path
-    except (OSError, PermissionError):
+    except (OSError, PermissionError) as error:
         LOG.error(
             (
-                "Error while creating the log directory. "
+                "Encountered an {error} while creating the log directory. "
-                "Please check the access rights for: '{}'"
+                "Please check the access rights for: '{log_path}'"
-            ).format(log_path)
+            ).format(
-        )
+                error=error,
+                log_path=log_path))
+
         # Fallback in default path if log_path != from constants path
         if log_path != constants.VALIDATIONS_LOG_BASEDIR:
-            create_log_dir(constants.VALIDATIONS_LOG_BASEDIR)
+            LOG.debug(
+                (
+                    "Resorting to the preset '{default_log_path}'."
+                ).format(
+                    default_log_path=constants.VALIDATIONS_LOG_BASEDIR))
+
+            return create_log_dir()
         raise RuntimeError()
 
 
 def create_artifacts_dir(log_path=constants.VALIDATIONS_LOG_BASEDIR,
-                         prefix=None):
+                         prefix=''):
-    """Create Ansible artifacts directory
+    """Create Ansible artifacts directory for the validation run
     :param log_path: Directory asbolute path
     :type log_path: `string`
     :param prefix: Playbook name
     :type prefix: `string`
-    :return: The UUID of the validation and the absolute path of the log file
+    :return: UUID of the validation run, absolute path of the validation artifacts directory
     :rtype: `string`, `string`
     """
     artifact_dir = os.path.join(log_path, 'artifacts')
     validation_uuid = str(uuid.uuid4())
-    log_dir = "{}/{}_{}_{}".format(artifact_dir, validation_uuid,
+    validation_artifacts_dir = "{}/{}_{}_{}".format(
-                                   (prefix if prefix else ''), current_time())
+        artifact_dir,
+        validation_uuid,
+        prefix,
+        current_time())
     try:
-        os.makedirs(log_dir)
+        os.makedirs(validation_artifacts_dir)
-        return validation_uuid, log_dir
+        return validation_uuid, validation_artifacts_dir
     except (OSError, PermissionError):
         LOG.exception(
             (
-                "Error while creating Ansible artifacts directory. "
+                "Error while creating Ansible artifacts log file. "
-                "Please check the access rights for: '{}'"
+                "Please check the access rights for '{}'"
-            ).format(log_dir)
+            ).format(validation_artifacts_dir))
-        )
 
         raise RuntimeError()