Repository: ansible/tower-cli
Pull request #270 (2017-03-13 16:26:37)

Use safer format indicator across base.py
connect #269.

It's terrifying to think such a bug has been lurking in base.py for so long: [Our `pk` argument is initialized as a string](https://github.com/ansible/tower-cli/blob/master/tower_cli/models/base.py#L263), yet `%d` is used to represent `pk` across various places in base.py. It is okay if `pk` is substituted by a real integer beforehand, but will cause traceback if it keeps what it is.

A safer way is substituting `%s` over `%d`, which accepts integer types as well. Guess the initial purpose of using `%d` is for type check, but a server-side error is way better than a traceback *_*


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 9eda91f..1e99876 100644
--- a/tower_cli/commands/version.py
+++ b/tower_cli/commands/version.py
@@ -20,13 +20,12 @@ from requests.exceptions import RequestException
 
 from tower_cli import __version__
 from tower_cli.api import client
-from tower_cli.utils.decorators import with_global_options
+from tower_cli.utils.decorators import command
 from tower_cli.utils.exceptions import TowerCLIError
 
 
-@click.command()
-@with_global_options
-def version(**kwargs):
+@command
+def version():
     """Display version information."""
 
     # Print out the current version of Tower CLI.
diff --git a/tower_cli/conf.py b/tower_cli/conf.py
index 6bd3b89..e230c00 100644
--- a/tower_cli/conf.py
+++ b/tower_cli/conf.py
@@ -71,15 +71,6 @@ 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`.
@@ -99,10 +90,12 @@ class Settings(object):
             'description_on': 'false',
             'certificate': '',
         }
-        self._defaults = self._new_parser(defaults=defaults)
+        self._defaults = Parser(defaults=defaults)
+        self._defaults.add_section('general')
 
         # If there is a global settings file, initialize it.
-        self._global = self._new_parser()
+        self._global = Parser()
+        self._global.add_section('general')
         if os.path.isdir('/etc/tower/'):
             # Sanity check: Try to get a list of files in `/etc/tower/`.
             #
@@ -129,14 +122,16 @@ class Settings(object):
             self._global.read('/etc/tower/tower_cli.cfg')
 
         # Initialize a parser for the user settings file.
-        self._user = self._new_parser()
+        self._user = Parser()
+        self._user.add_section('general')
 
         # 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 = self._new_parser()
+        self._local = Parser()
+        self._local.add_section('general')
 
         # If there is a local settings file in the current working directory
         # or any parent, read it into the parser object.
@@ -164,7 +159,8 @@ class Settings(object):
             self._local.read(local_filename)
 
         # Put a stubbed runtime parser in.
-        self._runtime = self._new_parser()
+        self._runtime = Parser()
+        self._runtime.add_section('general')
 
     def __getattr__(self, key):
         """Return the approprate value, intelligently type-casted in the
@@ -214,24 +210,6 @@ 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 4c40a3e..f92062b 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
+from __future__ import absolute_import, division, unicode_literals
 
 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 with_global_options
+from tower_cli.utils.decorators import command
 
 
 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 = with_global_options(self._echo_method(method))
+                new_method = self._echo_method(method)
 
                 # Soft copy the "__click_params__", if any exist.
                 # This is the internal holding method that the click library
@@ -246,14 +246,12 @@ class BaseResource(six.with_metaclass(ResourceMeta)):
                             help=option_help,
                             type=field.type,
                             show_default=field.show_default,
-                            multiple=field.multiple,
-                            is_eager=False
+                            multiple=field.multiple
                         )(new_method)
 
                 # Make a click Command instance using this method
                 # as the callback, and return it.
-                cmd = click.command(name=name, cls=Command, **attrs)(
-                    new_method)
+                cmd = command(name=name, cls=Command, **attrs)(new_method)
 
                 # If this method has a `pk` positional argument,
                 # then add a click argument for it.
@@ -694,7 +692,7 @@ class ResourceMethods(BaseResource):
         # If it turns out the record doesn't exist, handle the 404
         # appropriately (this is an okay response if `fail_on_missing` is
         # False).
-        url = '%s%d/' % (self.endpoint, pk)
+        url = '%s%s/' % (self.endpoint, pk)
         debug.log('DELETE %s' % url, fg='blue', bold=True)
         try:
             client.delete(url)
@@ -891,7 +889,7 @@ class MonitorableResource(ResourceMethods):
         Internal utility function to return standard out
         requires the pk of a unified job
         """
-        stdout_url = '%s%d/stdout/' % (self.unified_job_type, pk)
+        stdout_url = '%s%s/stdout/' % (self.unified_job_type, pk)
         payload = {
             'format': 'json', 'content_encoding': 'base64',
             'content_format': 'ansi'}
@@ -1145,7 +1143,7 @@ class ExeResource(MonitorableResource):
         # Get the job from Ansible Tower if pk given
         else:
             debug.log('Asking for job status.', header='details')
-            finished_endpoint = '%s%d/' % (self.endpoint, pk)
+            finished_endpoint = '%s%s/' % (self.endpoint, pk)
             job = client.get(finished_endpoint).json()
 
         # In most cases, we probably only want to know the status of the job
@@ -1175,7 +1173,7 @@ class ExeResource(MonitorableResource):
             existing_data = self.get(**kwargs)
             pk = existing_data['id']
 
-        cancel_endpoint = '%s%d/cancel/' % (self.endpoint, pk)
+        cancel_endpoint = '%s%s/cancel/' % (self.endpoint, pk)
         # Attempt to cancel the job.
         try:
             client.post(cancel_endpoint)
diff --git a/tower_cli/utils/decorators.py b/tower_cli/utils/decorators.py
index 08b7668..e15d003 100644
--- a/tower_cli/utils/decorators.py
+++ b/tower_cli/utils/decorators.py
@@ -15,33 +15,52 @@
 
 from __future__ import absolute_import
 
-import click
+import functools
+import types
 
-from functools import wraps
+import click
 
 from tower_cli.conf import settings
 
 
-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 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 with_global_options(method):
@@ -58,24 +77,21 @@ 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, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(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, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(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, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(method)
 
     # Create a global verbose/debug option.
@@ -85,24 +101,21 @@ 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, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(method)
     method = click.option(
         '-v', '--verbose',
         default=None,
         help='Show information about requests being made.',
         is_flag=True,
-        required=False, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(method)
     method = click.option(
         '--description-on',
         default=None,
         help='Show description in human-formatted output.',
         is_flag=True,
-        required=False, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(method)
 
     # Create a global SSL warning option.
@@ -112,8 +125,7 @@ def with_global_options(method):
         help='Turn off insecure connection warnings. Set config verify_ssl '
              'to make this permanent.',
         is_flag=True,
-        required=False, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(method)
 
     # Create a custom certificate specification option.
@@ -122,13 +134,9 @@ 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, callback=_apply_runtime_setting,
-        is_eager=True
+        required=False,
     )(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 71b07d4..531f0e5 100644
--- a/tests/test_utils_decorators.py
+++ b/tests/test_utils_decorators.py
@@ -15,33 +15,35 @@
 
 import click
 
-from tests.compat import unittest
+from tower_cli.utils.decorators import command
 
-from tower_cli.utils import decorators
+from tests.compat import unittest
 
 
 class CommandTests(unittest.TestCase):
     """Establish that the @command decorator works as I expect,
     with and without a call signature.
     """
-    def test_command_with_call_signature(self):
-        """Establish that if we call the @click.command decorator with a call
-        signature, that it decorates the function appropriately.
+    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
-        @click.command()
+        # Define a command.
+        @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 test_command_with_call_signature(self):
+        """Establish that if we call the @command decorator with a call
+        signature, that it decorates the function appropriately.
         """
-        def foo(**kwargs):
-            self.assertEqual(kwargs, {})
+        # Define a command
+        @command()
+        def foo():
+            pass
 
-        foo = decorators.runtime_context_manager(foo)
-        foo(tower_username='foobear')
+        # Ensure that it's a command.
+        self.assertIsInstance(foo, click.core.Command)