Repository: ansible/tower-cli
Pull request #266 (2017-03-09 03:05:02)

Change the ordering in which runtime config vars are processed
This is an extremely aggressive change, which gets rid of an old multi-purpose decorator (which was confusingly named "command", like many other command decorators), and moves one of its functions (the application of runtime parameters) into a callback method. Also relies on `is_eager`.

One shortcoming might be the removal of the contextmanager, which could mean that the values are "sticky" from one operation to another if used in a python context. I think the right solution going from there would be to just do some post-action hook of nuking the runtime config parser.

This should resolve a lot of reported issues simultaneously, so I'm not going to go about listing them all just yet. But there are some serious release planning considerations, and I want to give @wenottingham an idea that, no, this is not at all a simple fix, and merging this will cary substantial risks of unforeseen bugs, although the bug this resolves is likewise rather severe.

We probably want to exclude this for 3.1.0. I won't be able to do the necessary followup work (you can bet some tests will fail) until after next week. I'd vote to ship this this in a quick 3.1.1.

For the record, this problem also looks like it has existed for the vast majority of the history of tower-cli.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/10509370/badge)](https://coveralls.io/builds/10509370)

Coverage decreased (-0.3%) to 91.66% when pulling **5c1adf437fbbb8aa079eb1e7bc2c3ad2dc6408a3 on AlanCoding:config_fix** into **8eff993f7065f98697a3ff060d225ae6a55da753 on ansible:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/10509914/badge)](https://coveralls.io/builds/10509914)

Coverage decreased (-0.1%) to 91.819% when pulling **ce71346fe50ac360af9182fb9d6f38780016ffa0 on AlanCoding:config_fix** into **8eff993f7065f98697a3ff060d225ae6a55da753 on ansible:master**.

AlanCoding: Some more considerations are coming to light...

I added in some code to no longer make them "sticky", such that any existing runtime parameters are removed when invoking a command.

I have noticed that with this change, the settings get passed into `kwargs` for any arbitrary command. This will probably have undesirable effects, so this is a big TBD. Unfortunately, I can't think of a good way to resolve this except for re-introducing another decorator like the one I just removed. However, the contextmanager won't work for the original stated reasons here. The thing we really need is a post-command hook to destroy the runtime settings. Previously, the context manager would save the old runtime settings and reapply them after the command. To me, this is a bridge too far because what are "runtime" parameters other than parameters that don't persist?

The resolution order of decorators are confusing here, because click manages the order of processing of its own options, however, it still seems that a custom decorator can still be implemented and will be next-in-line _after_ click option processing. This was implicit in the previous runtime settings context manager, although you couldn't call it logically consistent considering that some options should have required the settings to already be set.

We also have this warning:

```
/Users/alancoding/.virtualenvs/cli2/lib/python2.7/site-packages/ansible_tower_cli-3.1.0-py2.7.egg/tower_cli/models/base.py:256: Warning: Click detected the use of the unicode_literals __future__ import.  This is heavily discouraged because it can introduce subtle bugs in your code.  You should instead use explicit u"" literals for your unicode strings.  For more information see http://click.pocoo.org/python3/
  new_method)
```

http://click.pocoo.org/5/python3/

Maybe we can simply apply the suggestion like `io.BytesIO(input.encode('utf-8'))` to something, but I'm still not sure why we see this now when we didn't before.
AlanCoding: Ok, pushed a fix for the biggest of the issues I mentioned. The settings options are no longer passed into the methods themselves, and I destroy the runtime settings entirely at the end of a command.
AlanCoding: Well, I just outright removed the import of `unicode_literals` from base.py, and everything seems to still be working correctly, even giving unicode fields as options.

I'm pretty confident in saying that I know why we hit the warning now, but not earlier. The warning is triggered if we import that _inside of the file that applies the command decorator from click_. In my changes, I moved the application of the decorator from the utils to base.py. The utils file did not have the import, thus the change. It was presumably a bad practice in all locations where it was applied, which includes a vast array of files throughout the code base.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/10511357/badge)](https://coveralls.io/builds/10511357)

Coverage decreased (-0.09%) to 91.85% when pulling **43239038fc1dc283bfb804a681a395108421dc76 on AlanCoding:config_fix** into **8eff993f7065f98697a3ff060d225ae6a55da753 on ansible:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/10511357/badge)](https://coveralls.io/builds/10511357)

Coverage decreased (-0.09%) to 91.85% when pulling **43239038fc1dc283bfb804a681a395108421dc76 on AlanCoding:config_fix** into **8eff993f7065f98697a3ff060d225ae6a55da753 on ansible:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/10514800/badge)](https://coveralls.io/builds/10514800)

Coverage decreased (-0.09%) to 91.85% when pulling **0054c57155fa18e79f9aebb6cd15462bb800fae1 on AlanCoding:config_fix** into **8eff993f7065f98697a3ff060d225ae6a55da753 on ansible:master**.


Patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index e3ae81e..a5e97d7 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -1,7 +1,7 @@
 Release History
 ===============
 
-3.1.0 (2017-03-09)
+3.1.0 (2017-03-08)
 ------------------
 
 - Improved job monitoring functionality to enable standard out streaming, which
diff --git a/tower_cli/commands/version.py b/tower_cli/commands/version.py
index 1e99876..9eda91f 100644
--- a/tower_cli/commands/version.py
+++ b/tower_cli/commands/version.py
@@ -20,12 +20,13 @@ from requests.exceptions import RequestException
 
 from tower_cli import __version__
 from tower_cli.api import client
-from tower_cli.utils.decorators import command
+from tower_cli.utils.decorators import with_global_options
 from tower_cli.utils.exceptions import TowerCLIError
 
 
-@command
-def version():
+@click.command()
+@with_global_options
+def version(**kwargs):
     """Display version information."""
 
     # Print out the current version of Tower CLI.
diff --git a/tower_cli/conf.py b/tower_cli/conf.py
index e230c00..6bd3b89 100644
--- a/tower_cli/conf.py
+++ b/tower_cli/conf.py
@@ -71,6 +71,15 @@ class Settings(object):
     """
     _parser_names = ['runtime', 'local', 'user', 'global', 'defaults']
 
+    @staticmethod
+    def _new_parser(defaults=None):
+        if defaults:
+            p = Parser(defaults=defaults)
+        else:
+            p = Parser()
+        p.add_section('general')
+        return p
+
     def __init__(self):
         """Create the settings object, and read from appropriate files as
         well as from `sys.argv`.
@@ -90,12 +99,10 @@ class Settings(object):
             'description_on': 'false',
             'certificate': '',
         }
-        self._defaults = Parser(defaults=defaults)
-        self._defaults.add_section('general')
+        self._defaults = self._new_parser(defaults=defaults)
 
         # If there is a global settings file, initialize it.
-        self._global = Parser()
-        self._global.add_section('general')
+        self._global = self._new_parser()
         if os.path.isdir('/etc/tower/'):
             # Sanity check: Try to get a list of files in `/etc/tower/`.
             #
@@ -122,16 +129,14 @@ class Settings(object):
             self._global.read('/etc/tower/tower_cli.cfg')
 
         # Initialize a parser for the user settings file.
-        self._user = Parser()
-        self._user.add_section('general')
+        self._user = self._new_parser()
 
         # If there is a user settings file, read it into the parser object.
         user_filename = os.path.expanduser('~/.tower_cli.cfg')
         self._user.read(user_filename)
 
         # Initialize a parser for the local settings file.
-        self._local = Parser()
-        self._local.add_section('general')
+        self._local = self._new_parser()
 
         # If there is a local settings file in the current working directory
         # or any parent, read it into the parser object.
@@ -159,8 +164,7 @@ class Settings(object):
             self._local.read(local_filename)
 
         # Put a stubbed runtime parser in.
-        self._runtime = Parser()
-        self._runtime.add_section('general')
+        self._runtime = self._new_parser()
 
     def __getattr__(self, key):
         """Return the approprate value, intelligently type-casted in the
@@ -210,6 +214,24 @@ class Settings(object):
         """
         return tuple([getattr(self, '_%s' % i) for i in self._parser_names])
 
+    def set_or_reset_runtime_param(self, key, value):
+        """Maintains the context of the runtime settings for invoking
+        a command.
+
+        This should be called by a click.option callback, and only
+        called once for each setting for each command invocation.
+
+        If the setting exists, it follows that the runtime settings are
+        stale, so the entire runtime settings are reset.
+        """
+        if self._runtime.has_option('general', key):
+            self._runtime = self._new_parser()
+
+        if value is None:
+            return
+        settings._runtime.set('general', key.replace('tower_', ''),
+                              six.text_type(value))
+
     @contextlib.contextmanager
     def runtime_values(self, **kwargs):
         """Temporarily override the runtime settings, which exist at the
diff --git a/tower_cli/models/base.py b/tower_cli/models/base.py
index 099a678..4c40a3e 100644
--- a/tower_cli/models/base.py
+++ b/tower_cli/models/base.py
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from __future__ import absolute_import, division, unicode_literals
+from __future__ import absolute_import, division
 
 import functools
 import inspect
@@ -39,7 +39,7 @@ from tower_cli.models.fields import Field
 from tower_cli.utils import exceptions as exc, parser, debug, secho
 from tower_cli.utils.command import Command
 from tower_cli.utils.data_structures import OrderedDict
-from tower_cli.utils.decorators import command
+from tower_cli.utils.decorators import with_global_options
 
 
 class ResourceMeta(type):
@@ -187,7 +187,7 @@ class BaseResource(six.with_metaclass(ResourceMeta)):
 
                 # Wrap the method, such that it outputs its final return
                 # value rather than returning it.
-                new_method = self._echo_method(method)
+                new_method = with_global_options(self._echo_method(method))
 
                 # Soft copy the "__click_params__", if any exist.
                 # This is the internal holding method that the click library
@@ -246,12 +246,14 @@ class BaseResource(six.with_metaclass(ResourceMeta)):
                             help=option_help,
                             type=field.type,
                             show_default=field.show_default,
-                            multiple=field.multiple
+                            multiple=field.multiple,
+                            is_eager=False
                         )(new_method)
 
                 # Make a click Command instance using this method
                 # as the callback, and return it.
-                cmd = command(name=name, cls=Command, **attrs)(new_method)
+                cmd = click.command(name=name, cls=Command, **attrs)(
+                    new_method)
 
                 # If this method has a `pk` positional argument,
                 # then add a click argument for it.
diff --git a/tower_cli/utils/decorators.py b/tower_cli/utils/decorators.py
index e15d003..08b7668 100644
--- a/tower_cli/utils/decorators.py
+++ b/tower_cli/utils/decorators.py
@@ -15,52 +15,33 @@
 
 from __future__ import absolute_import
 
-import functools
-import types
-
 import click
 
+from functools import wraps
+
 from tower_cli.conf import settings
 
 
-def command(method=None, **kwargs):
-    """Cause the given function to become a click command, and add all
-    global options.
-    """
-    # Define the actual decorator.
-    # This is done in such a way as to allow @command, @command(), and
-    # @command(foo='bar') to all work.
-    def actual_decorator(method):
-        # Create a wrapper function that will "eat" the authentication
-        # if it's provided as keyword arguments and apply it to settings.
-        @with_global_options
-        @click.command(**kwargs)
-        @functools.wraps(method)
-        def answer(*inner_a, **inner_kw):
-            runtime_settings = {
-                'host': inner_kw.pop('tower_host', None),
-                'password': inner_kw.pop('tower_password', None),
-                'format': inner_kw.pop('format', None),
-                'username': inner_kw.pop('tower_username', None),
-                'verbose': inner_kw.pop('verbose', None),
-                'description_on': inner_kw.pop('description_on', None),
-                'insecure': inner_kw.pop('insecure', None),
-                'certificate': inner_kw.pop('certificate', None)
-            }
-            with settings.runtime_values(**runtime_settings):
-                return method(*inner_a, **inner_kw)
-
-        # Done, return the wrapped-wrapped-wrapped-wrapped method.
-        # BECAUSE WE WRAP ALL THE THINGS!
-        return answer
-
-    # If we got the method straight-up, apply the decorator and return
-    # the decorated method; otherwise, return the actual decorator for
-    # the Python interpreter to apply.
-    if method and isinstance(method, types.FunctionType):
-        return actual_decorator(method)
-    else:
-        return actual_decorator
+def _apply_runtime_setting(ctx, param, value):
+    settings.set_or_reset_runtime_param(param.name, value)
+
+
+SETTINGS_PARMS = set([
+    'tower_host', 'tower_password', 'format', 'tower_username', 'verbose',
+    'description_on', 'insecure', 'certificate'
+])
+
+
+def runtime_context_manager(method):
+    @wraps(method)
+    def method_with_context_managed(*args, **kwargs):
+        # Remove the settings before running the method
+        for key in SETTINGS_PARMS:
+            kwargs.pop(key, None)
+        method(*args, **kwargs)
+        # Destroy the runtime settings
+        settings._runtime = settings._new_parser()
+    return method_with_context_managed
 
 
 def with_global_options(method):
@@ -77,21 +58,24 @@ def with_global_options(method):
              'HTTPS is assumed as the protocol unless "http://" is explicitly '
              'provided. This will take precedence over a host provided to '
              '`tower config`, if any.',
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
     method = click.option(
         '-u', '--tower-username',
         help='Username to use to authenticate to Ansible Tower. '
              'This will take precedence over a username provided to '
              '`tower config`, if any.',
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
     method = click.option(
         '-p', '--tower-password',
         help='Password to use to authenticate to Ansible Tower. '
              'This will take precedence over a password provided to '
              '`tower config`, if any.',
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
 
     # Create a global verbose/debug option.
@@ -101,21 +85,24 @@ def with_global_options(method):
              'reading output on the CLI; the "json" and "yaml" formats '
              'provide more data.',
         type=click.Choice(['human', 'json', 'yaml']),
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
     method = click.option(
         '-v', '--verbose',
         default=None,
         help='Show information about requests being made.',
         is_flag=True,
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
     method = click.option(
         '--description-on',
         default=None,
         help='Show description in human-formatted output.',
         is_flag=True,
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
 
     # Create a global SSL warning option.
@@ -125,7 +112,8 @@ def with_global_options(method):
         help='Turn off insecure connection warnings. Set config verify_ssl '
              'to make this permanent.',
         is_flag=True,
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
 
     # Create a custom certificate specification option.
@@ -134,9 +122,13 @@ def with_global_options(method):
         default=None,
         help='Path to a custom certificate file that will be used throughout'
              ' the command. Overwritten by --insecure flag if set.',
-        required=False,
+        required=False, callback=_apply_runtime_setting,
+        is_eager=True
     )(method)
 
+    # Manage the runtime settings context
+    method = runtime_context_manager(method)
+
     # Okay, we're done adding options; return the method.
     return method
 
diff --git a/tox.ini b/tox.ini
index a75c4b7..b80dde3 100644
--- a/tox.ini
+++ b/tox.ini
@@ -6,6 +6,7 @@ deps =
     -r{toxinidir}/requirements.txt
     -r{toxinidir}/tests/requirements.txt
 
+
 [testenv:py27]
 deps =
     {[testenv]deps}
@@ -18,7 +19,3 @@ deps =
     ordereddict
     simplejson
     unittest2
-
-[testenv:flake8]
-deps = flake8
-commands = flake8 {toxinidir}


Test patch:
diff --git a/tests/test_utils_decorators.py b/tests/test_utils_decorators.py
index 531f0e5..71b07d4 100644
--- a/tests/test_utils_decorators.py
+++ b/tests/test_utils_decorators.py
@@ -15,35 +15,33 @@
 
 import click
 
-from tower_cli.utils.decorators import command
-
 from tests.compat import unittest
 
+from tower_cli.utils import decorators
+
 
 class CommandTests(unittest.TestCase):
     """Establish that the @command decorator works as I expect,
     with and without a call signature.
     """
-    def test_command_without_call_signature(self):
-        """Establish that if we call the @command decorator without a
-        call signature, that it decorates the function appropriately.
-        """
-        # Define a command.
-        @command
-        def foo():
-            pass
-
-        # Ensure that it's a command.
-        self.assertIsInstance(foo, click.core.Command)
-
     def test_command_with_call_signature(self):
-        """Establish that if we call the @command decorator with a call
+        """Establish that if we call the @click.command decorator with a call
         signature, that it decorates the function appropriately.
         """
         # Define a command
-        @command()
+        @click.command()
         def foo():
             pass
 
         # Ensure that it's a command.
         self.assertIsInstance(foo, click.core.Command)
+
+    def test_runtime_context_manager(self):
+        """Test that the kwargs from settings are removed before
+        running the command from the resource itself.
+        """
+        def foo(**kwargs):
+            self.assertEqual(kwargs, {})
+
+        foo = decorators.runtime_context_manager(foo)
+        foo(tower_username='foobear')