ARIA-148 CLI display commands #107

tliron · 2017-04-20T22:56:32Z

Also includes some typo fixes for the CLI.

asfbot · 2017-04-20T22:56:33Z

Can one of the admins verify this patch?

ran-z · 2017-05-03T14:07:07Z

aria/cli/helptexts.py

@@ -47,3 +47,8 @@
 SORT_BY = "Key for sorting the list"
 DESCENDING = "Sort list in descending order [default: False]"
 JSON_OUTPUT = "Output logs in a consumable JSON format"
+
+DISPLAY_JSON = "Display in JSON format"


lets merge this with JSON_OUTPUT,
the latter is a good name, and simply have it as "Format the output as JSON"

ran-z · 2017-05-03T14:07:20Z

aria/cli/core/aria.py

@@ -325,6 +325,30 @@ def __init__(self):
            default=defaults.SERVICE_TEMPLATE_FILENAME,
            help=helptexts.SERVICE_TEMPLATE_FILENAME)

+        self.display_json = click.option(


merge this with the other JSON one

ran-z · 2017-05-03T14:07:35Z

aria/cli/core/aria.py

+            is_flag=True,
+            help=helptexts.DISPLAY_JSON)
+
+        self.display_yaml = click.option(


yaml_output?

ran-z · 2017-05-03T14:11:07Z

aria/cli/commands/services.py

+    logger.info('Displaying service {0}...'.format(service_name))
+    service = model_storage.service.get_by_name(service_name)
+    consumption.ConsumptionContext()
+    if graph:


General reminder that none of the methods below are standard for the CLI
Fine for now, but keep https://issues.apache.org/jira/browse/ARIA-183 in mind

ran-z · 2017-05-03T14:11:42Z

aria/cli/commands/services.py

+@aria.options.display_graph
+@aria.pass_model_storage
+@aria.pass_logger
+def display(service_name, model_storage, json, yaml, graph, logger):


nitpicking that model_storage should probably sit after graph, before logger

ran-z · 2017-05-03T14:17:02Z

aria/cli/commands/service_templates.py

+@aria.options.display_types
+@aria.pass_model_storage
+@aria.pass_logger
+def display(service_template_name, model_storage, json, yaml, types, logger):


Having both display and show is confusing :/
I agree however that it doesn't make sense to make this command into a mere flag of show as they're very different.
I'm generally ok with this as is, but Maxim wanted me to suggest aria parse service/service-template as an option as well. I'll leave it for your consideration :)

ran-z · 2017-05-10T12:28:05Z

aria/cli/table.py

@@ -85,7 +85,7 @@ def get_values_per_column(column, row_data):

            return val
        else:
-            return defaults[column]
+            return defaults.get(column) if defaults is not None else None


should instead have a defaults = defaults or {} before calling the nested method

ran-z · 2017-05-10T12:28:43Z

aria/cli/commands/services.py

+    service = model_storage.service.get_by_name(service_name)
+
+    if json or yaml:
+        all = True


i'd rename the flag, it's both unclear and a python keyword. show_all or all_info might be better.

i actually didnt mean the flag but only the variables etc.,
although now that i think of it it might also be better to change the flag, as it might be confusing i.e. users might think it will show all service-templates or so..?
in any case what matters to me more is the variables, the flag can still be all to the user, but the "destination" of the variable (configurable in aria.py) should be renamed.

ran-z · 2017-05-10T12:39:30Z

aria/cli/commands/services.py

+    service = model_storage.service.get_by_name(service_name)
+
+    if json or yaml:
+        all = True


i actually didnt mean the flag but only the variables etc.,
although now that i think of it it might also be better to change the flag, as it might be confusing i.e. users might think it will show all service-templates or so..?
in any case what matters to me more is the variables, the flag can still be all to the user, but the "destination" of the variable (configurable in aria.py) should be renamed.

ran-z · 2017-05-10T12:42:58Z

aria/cli/commands/service_templates.py

-        logger.info('Existing services:')
-        for service in service_template.services:
-            logger.info('\t{0}'.format(service.name))
+    if json or yaml:


i'm a bit confused by this. so yaml, json, and all all mean all,
and json and yaml can be used simulatniously but only json will get printed.

Re the latter problem, please have a look at aria.py for how to make flags mutually-exclusive.
re the former, it might be better to also have the all flag as mutually-exclusive with the other two, (or that possibly using either json or yaml requires the all flag as well..?)

ran-z · 2017-05-11T08:43:05Z

aria/cli/core/aria.py

+            '--full',
+            'mode',
+            is_flag=True,
+            flag_value='full',


I think I don't like this approach as much at the previous one.
It seems like it simplifies the code a bit but makes things somewhat more confusing to the user - specifically the fact that only the last flag of a given "destination" (e.g. mode) takes hold.

If instead you have different destinations for these flags, it'd be easier to inform the user of misuse (even if for some reason the mutually-exclusive mechanism is problematic to use, you can always just manually validate the parameters in the command function itself)

ran-z · 2017-05-11T08:44:20Z

aria/cli/core/aria.py

    def __init__(self, *args, **kwargs):
-        self.mutually_exclusive = set(kwargs.pop('mutually_exclusive', []))
+        self.mutually_exclusive = kwargs.pop('mutually_exclusive', tuple())


why change this from a set to another type?

ran-z · 2017-05-11T08:44:43Z

aria/cli/core/aria.py

@@ -65,15 +58,33 @@ def __init__(self, *args, **kwargs):
        super(MutuallyExclusiveOption, self).__init__(*args, **kwargs)

    def handle_parse_result(self, ctx, opts, args):
-        if self.mutually_exclusive.intersection(opts) and self.name in opts:
+        if (self.name in opts) and self.mutually_exclusive.keys().intersection(opts):


seems like mutually_exclusive might be a tuple now, so .keys() would raise an error here.

ran-z · 2017-05-11T08:45:13Z

aria/cli/core/aria.py

+
+def mutually_exclusive_option(*param_decls, **attrs):
+    """
+    Makes options mutually exclusive. The decorator must pass a a ``mutually_exclusive`` argument


ran-z · 2017-05-11T08:47:51Z

aria/cli/core/aria.py

+    options.
+    """
+    def decorator(func):
+        if 'help' in attrs:


could you please document this func a bit more?
I'm not sure what are param_decls and attrs in this case, why help is being inspected, and why the internal __click_params__ is accessed directly here.

* Allow "--full" flag to provide a complete dump * Allow "--json" and "--yaml" flags for dump in those formats * Support for node graph and type hierarchies * Some fixes for YAML dump for our custom types * Also closes ARIA-186: "aria services show" command

asfgit force-pushed the ARIA-148-extra-cli-commands branch from cd5f233 to cf08b4b Compare April 20, 2017 23:03

hubot deleted the ARIA-148-extra-cli-commands branch April 28, 2017 17:09

hubot restored the ARIA-148-extra-cli-commands branch April 28, 2017 22:05

ran-z suggested changes May 3, 2017

View reviewed changes

asfgit force-pushed the ARIA-148-extra-cli-commands branch 4 times, most recently from 7442a76 to 7e75948 Compare May 9, 2017 21:03

ran-z suggested changes May 10, 2017

View reviewed changes

asfgit force-pushed the ARIA-148-extra-cli-commands branch 2 times, most recently from 990a792 to 8219354 Compare May 11, 2017 02:01

ran-z suggested changes May 11, 2017

View reviewed changes

asfgit force-pushed the ARIA-148-extra-cli-commands branch from 8219354 to fdd57c4 Compare May 11, 2017 17:55

asfgit merged commit fdd57c4 into master May 11, 2017

asfgit deleted the ARIA-148-extra-cli-commands branch May 14, 2017 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARIA-148 CLI display commands #107

ARIA-148 CLI display commands #107

tliron commented Apr 20, 2017

asfbot commented Apr 20, 2017

ran-z May 3, 2017

ran-z May 3, 2017

ran-z May 3, 2017

ran-z May 3, 2017

ran-z May 3, 2017

ran-z May 3, 2017

ran-z May 10, 2017

ran-z May 10, 2017

ran-z May 10, 2017

ran-z May 10, 2017

ran-z May 10, 2017 •

edited

ran-z May 11, 2017

ran-z May 11, 2017

ran-z May 11, 2017

ran-z May 11, 2017

ran-z May 11, 2017

ARIA-148 CLI display commands #107

ARIA-148 CLI display commands #107

Conversation

tliron commented Apr 20, 2017

asfbot commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ran-z May 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ran-z May 10, 2017 •

edited