Merge pull request #4922 from sosiouxme/20170728-improve-get-vars

openshift_checks: enable variable conversion

3 files changed, 73 insertions, 7 deletions

diff --git a/roles/openshift_health_checker/openshift_checks/__init__.py b/roles/openshift_health_checker/openshift_checks/__init__.py
index f26008c9f..09139408c 100644
--- a/roles/openshift_health_checker/openshift_checks/__init__.py
+++ b/roles/openshift_health_checker/openshift_checks/__init__.py
@@ -10,6 +10,7 @@ from importlib import import_module
 
 from ansible.module_utils import six
 from ansible.module_utils.six.moves import reduce  # pylint: disable=import-error,redefined-builtin
+from ansible.plugins.filter.core import to_bool as ansible_to_bool
 
 
 class OpenShiftCheckException(Exception):
@@ -119,16 +120,59 @@ class OpenShiftCheck(object):
 
         Ansible task_vars structures are Python dicts, often mapping strings to
         other dicts. This helper makes it easier to get a nested value, raising
-        OpenShiftCheckException when a key is not found or returning a default value
-        provided as a keyword argument.
+        OpenShiftCheckException when a key is not found.
+
+        Keyword args:
+          default:
+            On missing key, return this as default value instead of raising exception.
+          convert:
+            Supply a function to apply to normalize the value before returning it.
+            None is the default (return as-is).
+            This function should raise ValueError if the user has provided a value
+            that cannot be converted, or OpenShiftCheckException if some other
+            problem needs to be described to the user.
         """
+        if len(keys) == 1:
+            keys = keys[0].split(".")
+
         try:
             value = reduce(operator.getitem, keys, self.task_vars)
         except (KeyError, TypeError):
-            if "default" in kwargs:
-                return kwargs["default"]
-            raise OpenShiftCheckException("'{}' is undefined".format(".".join(map(str, keys))))
-        return value
+            if "default" not in kwargs:
+                raise OpenShiftCheckException(
+                    "This check expects the '{}' inventory variable to be defined\n"
+                    "in order to proceed, but it is undefined. There may be a bug\n"
+                    "in Ansible, the checks, or their dependencies."
+                    "".format(".".join(map(str, keys)))
+                )
+            value = kwargs["default"]
+
+        convert = kwargs.get("convert", None)
+        try:
+            if convert is None:
+                return value
+            elif convert is bool:  # interpret bool as Ansible does, instead of python truthiness
+                return ansible_to_bool(value)
+            else:
+                return convert(value)
+
+        except ValueError as error:  # user error in specifying value
+            raise OpenShiftCheckException(
+                'Cannot convert inventory variable to expected type:\n'
+                '  "{var}={value}"\n'
+                '{error}'.format(var=".".join(keys), value=value, error=error)
+            )
+
+        except OpenShiftCheckException:  # some other check-specific problem
+            raise
+
+        except Exception as error:  # probably a bug in the function
+            raise OpenShiftCheckException(
+                'There is a bug in this check. While trying to convert variable \n'
+                '  "{var}={value}"\n'
+                'the given converter cannot be used or failed unexpectedly:\n'
+                '{error}'.format(var=".".join(keys), value=value, error=error)
+            )
 
     @staticmethod
     def get_major_minor_version(openshift_image_tag):
diff --git a/roles/openshift_health_checker/openshift_checks/logging/logging.py b/roles/openshift_health_checker/openshift_checks/logging/logging.py
index 3b7c39760..ecd8adb64 100644
--- a/roles/openshift_health_checker/openshift_checks/logging/logging.py
+++ b/roles/openshift_health_checker/openshift_checks/logging/logging.py
@@ -27,7 +27,7 @@ class LoggingCheck(OpenShiftCheck):
     name = "logging"
 
     def is_active(self):
-        logging_deployed = self.get_var("openshift_hosted_logging_deploy", default=False)
+        logging_deployed = self.get_var("openshift_hosted_logging_deploy", convert=bool, default=False)
         return logging_deployed and super(LoggingCheck, self).is_active() and self.is_first_master()
 
     def is_first_master(self):
diff --git a/roles/openshift_health_checker/test/openshift_check_test.py b/roles/openshift_health_checker/test/openshift_check_test.py
index 43aa875f4..789784c77 100644
--- a/roles/openshift_health_checker/test/openshift_check_test.py
+++ b/roles/openshift_health_checker/test/openshift_check_test.py
@@ -81,6 +81,7 @@ def dummy_check(task_vars):
 @pytest.mark.parametrize("keys,expected", [
     (("foo",), 42),
     (("bar", "baz"), "openshift"),
+    (("bar.baz",), "openshift"),
 ])
 def test_get_var_ok(task_vars, keys, expected):
     assert dummy_check(task_vars).get_var(*keys) == expected
@@ -94,3 +95,24 @@ def test_get_var_error(task_vars, missing_keys):
 def test_get_var_default(task_vars, missing_keys):
     default = object()
     assert dummy_check(task_vars).get_var(*missing_keys, default=default) == default
+
+
+@pytest.mark.parametrize("keys, convert, expected", [
+    (("foo",), str, "42"),
+    (("foo",), float, 42.0),
+    (("bar", "baz"), bool, False),
+])
+def test_get_var_convert(task_vars, keys, convert, expected):
+    assert dummy_check(task_vars).get_var(*keys, convert=convert) == expected
+
+
+@pytest.mark.parametrize("keys, convert", [
+    (("bar", "baz"), int),
+    (("bar.baz"), float),
+    (("foo"), "bogus"),
+    (("foo"), lambda a, b: 1),
+    (("foo"), lambda a: 1 / 0),
+])
+def test_get_var_convert_error(task_vars, keys, convert):
+    with pytest.raises(OpenShiftCheckException):
+        dummy_check(task_vars).get_var(*keys, convert=convert)