Repository: ansible/tower-cli
Pull request #317 (2017-07-06 19:07:00)

Categorize help text options
This PR increases readability of action command help text by categorizing options into 4 categories.

1. 'Field Options', which are options derived from resource fields.
2. 'Local Options', which are custom options specific to individual action commands.
3. 'Global Options', which are options that control runtime settings keyword arguments.
4. 'Other Options', which includes and only includes `--help` for now.

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

Coverage increased (+0.1%) to 89.658% when pulling **210b38bf184574a5e557b2a05605aab74d55b5c8 on jangsutsr:categorize_help_text_options** into **4fe170e79c19f95e72355fb6106970a96ab82a46 on ansible:master**.


Patch:
diff --git a/.coveragerc b/.coveragerc
index 370e951..b595b3f 100644
--- a/.coveragerc
+++ b/.coveragerc
@@ -16,4 +16,4 @@ exclude_lines =
 
     # Don't complain about missing debug-only code:
     def __repr__
-
+show_missing = True
diff --git a/docs/NOTIFICATION_TEMPLATE_MANAGEMENT.md b/docs/NOTIFICATION_TEMPLATE_MANAGEMENT.md
index 780a6c0..1e74dea 100644
--- a/docs/NOTIFICATION_TEMPLATE_MANAGEMENT.md
+++ b/docs/NOTIFICATION_TEMPLATE_MANAGEMENT.md
@@ -61,10 +61,10 @@ is provided.
 
 ```bash
 # Create a notification template in isolation.
-tower-cli notification_template create --name foo --description bar --notification-type slack --channels a --channels b --token hey --organization Default
+tower-cli notification_template create --name foo --description bar --notification-type slack --channels a --channels b --token hey
 
 # Create a notification template under an existing job template.
-tower-cli notification_template create --name foo --description bar --notification-type slack --channels a --channels b --token hey --job-template 5 --organization Default
+tower-cli notification_template create --name foo --description bar --notification-type slack --channels a --channels b --token hey --job-template 5
 
 # Get exactly one notification template.
 tower-cli notification_template get --name foo
diff --git a/tower_cli/cli/action.py b/tower_cli/cli/action.py
index f0f77c4..8f22848 100644
--- a/tower_cli/cli/action.py
+++ b/tower_cli/cli/action.py
@@ -14,6 +14,9 @@
 # limitations under the License.
 
 import click
+from click.formatting import join_options
+
+from tower_cli.conf import SETTINGS_PARMS
 
 
 class ActionSubcommand(click.Command):
@@ -41,3 +44,42 @@ class ActionSubcommand(click.Command):
             click.echo(ctx.get_help())
             ctx.exit()
         return super(ActionSubcommand, self).parse_args(ctx, args)
+
+    def format_options(self, ctx, formatter):
+        """Monkey-patch click's format_options method to support option categorization.
+        """
+        field_opts = []
+        global_opts = []
+        local_opts = []
+        other_opts = []
+        for param in self.params:
+            if param.name in SETTINGS_PARMS:
+                opts = global_opts
+            elif getattr(param, 'help', None) and param.help.startswith('[FIELD]'):
+                opts = field_opts
+                param.help = param.help[len('[FIELD]'):]
+            else:
+                opts = local_opts
+            rv = param.get_help_record(ctx)
+            if rv is None:
+                continue
+            else:
+                opts.append(rv)
+
+        if self.add_help_option:
+            help_options = self.get_help_option_names(ctx)
+            if help_options:
+                other_opts.append([join_options(help_options)[0], 'Show this message and exit.'])
+
+        if field_opts:
+            with formatter.section('Field Options'):
+                formatter.write_dl(field_opts)
+        if local_opts:
+            with formatter.section('Local Options'):
+                formatter.write_dl(local_opts)
+        if global_opts:
+            with formatter.section('Global Options'):
+                formatter.write_dl(global_opts)
+        if other_opts:
+            with formatter.section('Other Options'):
+                formatter.write_dl(other_opts)
diff --git a/tower_cli/cli/resource.py b/tower_cli/cli/resource.py
index 0a37e74..8872c5f 100644
--- a/tower_cli/cli/resource.py
+++ b/tower_cli/cli/resource.py
@@ -187,8 +187,7 @@ class ResSubcommand(click.MultiCommand):
         for col in columns:
             widths[col] = max(
                 len(col),
-                *[len(six.text_type(i.get(col, 'N/A')))
-                  for i in raw_rows]
+                *[len(six.text_type(i.get(col, 'N/A'))) for i in raw_rows]
             )
             fd = fields_by_name.get(col, None)
             if fd is not None and fd.col_width is not None:
@@ -327,10 +326,10 @@ class ResSubcommand(click.MultiCommand):
                 option_help = field.help
                 if field.required:
                     option_help = '[REQUIRED] ' + option_help
+                option_help = '[FIELD]' + option_help
                 click.option(
                     *args,
-                    default=field.default if not ignore_defaults
-                    else None,
+                    default=field.default if not ignore_defaults else None,
                     help=option_help,
                     type=field.type,
                     show_default=field.show_default,
@@ -340,15 +339,13 @@ class ResSubcommand(click.MultiCommand):
 
         # Make a click Command instance using this method
         # as the callback, and return it.
-        cmd = click.command(name=name, cls=ActionSubcommand, **attrs)(
-            new_method)
+        cmd = click.command(name=name, cls=ActionSubcommand, **attrs)(new_method)
 
         # If this method has a `pk` positional argument,
         # then add a click argument for it.
         code = six.get_function_code(method)
         if 'pk' in code.co_varnames:
-            click.argument('pk', nargs=1, required=False,
-                           type=str, metavar='[ID]')(cmd)
+            click.argument('pk', nargs=1, required=False, type=str, metavar='[ID]')(cmd)
 
         # Done; return the command.
         return cmd
diff --git a/tower_cli/resources/job_template.py b/tower_cli/resources/job_template.py
index 51ea555..34e6b03 100644
--- a/tower_cli/resources/job_template.py
+++ b/tower_cli/resources/job_template.py
@@ -18,8 +18,7 @@ from __future__ import absolute_import, unicode_literals
 import click
 
 from tower_cli import models, resources
-from tower_cli.utils import types, parser
-from tower_cli.api import client
+from tower_cli.utils import types
 
 
 class Resource(models.SurveyResource):
@@ -90,7 +89,6 @@ class Resource(models.SurveyResource):
         type=bool, required=False, display=False,
         help_text='Prompt user for machine credential on launch.')
     become_enabled = models.Field(type=bool, required=False, display=False)
-    allow_simultaneous = models.Field(type=bool, required=False, display=False)
     timeout = models.Field(type=int, required=False, display=False,
                            help_text='The timeout field (in seconds).')
     survey_enabled = models.Field(
@@ -147,18 +145,3 @@ class Resource(models.SurveyResource):
         """Disassociate a notification template from this job template."""
         return self._disassoc('notification_templates_%s' % status,
                               job_template, notification_template)
-
-    @resources.command(use_fields_as_options=('extra_vars'))
-    @click.option('--host-config-key', help='Job-template-specific string used to authenticate '
-                  'host during provisioning callback.')
-    def callback(self, pk=None, host_config_key='', extra_vars=None):
-        """Contact Tower and request a configuration update using this job template."""
-        url = self.endpoint + '%s/callback/' % pk
-        if not host_config_key:
-            host_config_key = client.get(url).json()['host_config_key']
-        post_data = {'host_config_key': host_config_key}
-        if extra_vars:
-            post_data['extra_vars'] = parser.process_extra_vars(list(extra_vars), force_json=True)
-        r = client.post(url, data=post_data, auth=None)
-        if r.status_code == 201:
-            return {'changed': True}
diff --git a/tower_cli/resources/notification_template.py b/tower_cli/resources/notification_template.py
index 98e13ca..ca5f0ef 100644
--- a/tower_cli/resources/notification_template.py
+++ b/tower_cli/resources/notification_template.py
@@ -124,11 +124,6 @@ class Resource(models.Resource):
                                             'gray', 'random']),
                          help_text='[{0}]The notification color.'.
                          format('hipchat'))
-    rooms = models.Field(required=False, display=False, default=False,
-                         help_text='[{0}]Rooms to send notification to. '
-                         'Use multiple flags to send to multiple rooms, ex '
-                         '--rooms=A --rooms=B'.
-                         format('hipchat'), multiple=True)
     notify = models.Field(required=False, display=False, default=False,
                           help_text='[{0}]The notify channel trigger.'.
                           format('hipchat'))
@@ -225,8 +220,8 @@ class Resource(models.Resource):
                     debug.log('Notification template already exists, '
                               'associating with job template.',
                               header='details')
-                    return jt.associate_notification_template(
-                        jt_id, nt_id, status=status)
+                    return jt.associate_notification(jt_id, nt_id,
+                                                     status=status)
             self.endpoint = '/job_templates/%d/notification_templates_%s/' %\
                             (jt_id, status)
         self._configuration(kwargs, config_item)


Test patch:
diff --git a/tests/test_cli_action.py b/tests/test_cli_action.py
index 8344f74..9f12bb4 100644
--- a/tests/test_cli_action.py
+++ b/tests/test_cli_action.py
@@ -21,6 +21,22 @@ from tower_cli.cli.action import ActionSubcommand
 from tests.compat import unittest
 
 
+CATEGORIZED_OUTPUT = """Usage: foo [OPTIONS]
+
+Field Options:
+  --bar TEXT  foobar
+
+Local Options:
+  --foo TEXT  foobar
+
+Global Options:
+  --tower-host TEXT  foobar
+
+Other Options:
+  --help  Show this message and exit.
+"""
+
+
 class ActionCommandTests(unittest.TestCase):
     """A set of tests to ensure that the tower_cli Command class works
     in the way we expect.
@@ -45,3 +61,23 @@ class ActionCommandTests(unittest.TestCase):
         result = self.runner.invoke(foo)
         self.assertIn('--help', result.output)
         self.assertIn('Show this message and exit.\n', result.output)
+
+    def test_categorize_options(self):
+        """Establish that options in help text are correctly categorized.
+        """
+        @click.command(cls=ActionSubcommand)
+        @click.option('--foo', help='foobar')
+        @click.option('--bar', help='[FIELD]foobar')
+        @click.option('--tower-host', help='foobar')
+        def foo():
+            pass
+
+        result = self.runner.invoke(foo)
+        self.assertEqual(result.output, CATEGORIZED_OUTPUT)
+
+        @click.command(cls=ActionSubcommand, add_help_option=False)
+        def bar():
+            pass
+
+        result = self.runner.invoke(bar)
+        self.assertEqual(result.output, 'Usage: bar [OPTIONS]\n')
diff --git a/tests/test_cli_resource.py b/tests/test_cli_resource.py
index 2b92e65..696c3bb 100644
--- a/tests/test_cli_resource.py
+++ b/tests/test_cli_resource.py
@@ -164,6 +164,19 @@ class SubcommandTests(unittest.TestCase):
         self.assertEqual(opt.name, 'internal_name')
         self.assertEqual(opt.opts, ['--option-name'])
 
+    def test_field_help_text_has_prefix(self):
+        """Establish that resource field help text is properly prefixed.
+        """
+        class FieldHelpTextResource(models.Resource):
+            endpoint = '/foobar/'
+
+            option_name = models.Field('internal_name', help_text='foobar', required=False)
+
+        cmd = ResSubcommand(FieldHelpTextResource()).get_command(None, 'get')
+
+        opt = cmd.params[0]
+        self.assertEqual(opt.help, '[FIELD]foobar')
+
     def test_docstring_replacement_an(self):
         """Establish that for resources with names beginning with vowels,
         that the automatic docstring replacement is gramatically correct.
diff --git a/tests/test_resources_job_template.py b/tests/test_resources_job_template.py
index 575cbee..79cc44f 100644
--- a/tests/test_resources_job_template.py
+++ b/tests/test_resources_job_template.py
@@ -182,12 +182,3 @@ class TemplateTests(unittest.TestCase):
             self.res.disassociate_notification_template(5, 3, 'any')
             self.assertEqual(t.requests[1].body,
                              json.dumps({'disassociate': True, 'id': 3}))
-
-    def test_callback(self):
-        """Establish that a job template should be able to conduct provisioning callback
-        """
-        with client.test_mode as t:
-            t.register_json('/job_templates/5/callback/', {'host_config_key': 'foobar'})
-            t.register_json('/job_templates/5/callback/', {}, method='POST')
-            self.res.callback(5)
-            self.assertEqual(t.requests[1].body, json.dumps({"host_config_key": "foobar"}))