reload hooks between uses (#227)

* reload hooks between uses

* fix test by ensuring that the mock_hooks module is always unimported

* reword docstring, add TODO to remove in next major release

Author: ITProKyle

CHANGELOG.md

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Fixed
+- global variables in hooks are now reloaded between uses to mimic functionality present in `>1.5.0`
 
 ## [1.6.0] - 2020-04-07
 ### Fixed

runway/cfngin/hooks/utils.py

@@ -64,7 +64,7 @@ def handle_hooks(stage, hooks, provider, context):  # pylint: disable=too-many-s
             continue
 
         try:
-            method = load_object_from_string(hook.path)
+            method = load_object_from_string(hook.path, try_reload=True)
         except (AttributeError, ImportError):
             LOGGER.exception("Unable to load method at %s:", hook.path)
             if required:

runway/util.py

@@ -344,11 +344,18 @@ def environ(env=None, **kwargs):
                 os.environ[key] = val
 
 
-def load_object_from_string(fqcn):
+def load_object_from_string(fqcn, try_reload=False):
     """Convert "." delimited strings to a python object.
 
-    Given a "." delimited string representing the full path to an object
-    (function, class, variable) inside a module, return that object.
+    Args:
+        fqcn (str): A "." delimited string representing the full path to an
+            object (function, class, variable) inside a module
+        try_reload (bool): Try to reload the module so any global variables
+            set within the file during import are reloaded. This only applies
+            to modules that are already imported and are not builtin.
+
+    Returns:
+        Any: Object being imported from the provided path.
 
     Example::
 
@@ -359,9 +366,20 @@ def load_object_from_string(fqcn):
     """
     module_path = "__main__"
     object_name = fqcn
-    if "." in fqcn:
-        module_path, object_name = fqcn.rsplit(".", 1)
-        importlib.import_module(module_path)
+    if '.' in object_name:
+        module_path, object_name = fqcn.rsplit('.', 1)
+        if (
+                try_reload and
+                sys.modules.get(module_path) and
+                module_path.split('.')[0] not in sys.builtin_module_names  # skip builtins
+        ):
+            # TODO remove is next major release; backport circumvents expected functionality
+            #
+            # pylint<2.3.1 incorrectly identifies this
+            # pylint: disable=too-many-function-args
+            six.moves.reload_module(sys.modules[module_path])
+        else:
+            importlib.import_module(module_path)
     return getattr(sys.modules[module_path], object_name)
 
 

tests/cfngin/hooks/test_utils.py

@@ -3,6 +3,8 @@
 import queue
 import unittest
 
+from mock import call, patch
+
 from runway.cfngin.config import Hook
 from runway.cfngin.hooks.utils import handle_hooks
 
@@ -49,14 +51,19 @@ def test_default_required_hook(self):
         with self.assertRaises(AttributeError):
             handle_hooks("missing", hooks, self.provider, self.context)
 
-    def test_valid_hook(self):
+    @patch('runway.cfngin.hooks.utils.load_object_from_string')
+    def test_valid_hook(self, mock_load):
         """Test valid hook."""
+        mock_load.side_effect = [mock_hook, MockHook]
         hooks = [
             Hook({"path": "tests.cfngin.hooks.test_utils.mock_hook",
                   "required": True}),
             Hook({'path': 'tests.cfngin.hooks.test_utils.MockHook',
                   'required': True})]
         handle_hooks('pre_build', hooks, self.provider, self.context)
+        assert mock_load.call_count == 2
+        mock_load.assert_has_calls([call(hooks[0].path, try_reload=True),
+                                    call(hooks[1].path, try_reload=True)])
         good = HOOK_QUEUE.get_nowait()
         self.assertEqual(good["provider"].region, "us-east-1")
         with self.assertRaises(queue.Empty):

tests/fixtures/__init__.py

@@ -0,0 +1 @@
+"""Empty module for python import traversal."""

tests/fixtures/mock_hooks.py

@@ -0,0 +1,4 @@
+"""Mock hooks."""
+import os
+
+GLOBAL_VALUE = os.getenv('AWS_DEFAULT_REGION')

tests/test_util.py

@@ -4,7 +4,7 @@
 import string
 import sys
 
-from mock import patch
+from mock import MagicMock, patch
 
 from runway.util import MutableMap, argv, environ, load_object_from_string
 
@@ -124,3 +124,36 @@ def test_load_object_from_string():
     )
     for test in tests:
         assert load_object_from_string(test[0]) is test[1]
+
+    obj_path = 'tests.fixtures.mock_hooks.GLOBAL_VALUE'
+    # check value from os.environ
+    assert load_object_from_string(obj_path, try_reload=True) == 'us-east-1'
+
+    with environ({'AWS_DEFAULT_REGION': 'us-west-2'}):
+        # check value from os.environ after changing it to ensure reload
+        assert load_object_from_string(obj_path, try_reload=True) == 'us-west-2'
+
+
+@patch('runway.util.six')
+def test_load_object_from_string_reload_conditions(mock_six):
+    """Test load_object_from_string reload conditions."""
+    mock_six.moves.reload_module.return_value = MagicMock()
+    builtin_test = 'sys.version_info'
+    mock_hook = 'tests.fixtures.mock_hooks.GLOBAL_VALUE'
+
+    try:
+        del sys.modules['tests.fixtures.mock_hooks']
+    except:  # noqa pylint: disable=bare-except
+        pass
+
+    load_object_from_string(builtin_test, try_reload=False)
+    mock_six.moves.reload_module.assert_not_called()
+
+    load_object_from_string(builtin_test, try_reload=True)
+    mock_six.moves.reload_module.assert_not_called()
+
+    load_object_from_string(mock_hook, try_reload=True)
+    mock_six.moves.reload_module.assert_not_called()
+
+    load_object_from_string(mock_hook, try_reload=True)
+    mock_six.moves.reload_module.assert_called_once()