Command linter #5952

williexu · 2018-03-29T19:39:10Z

Addresses: #734
(I'll make a new issue with just the rules list and close the issue above once this is merged)

-added doc help gen and loading of all commands to core util
-command linter rules can be one of: 'parameter_rule', 'command_rule', 'command_group_rule', 'help_file_entry_rule'

promptws · 2018-03-29T19:39:12Z

View a preview at https://prompt.ws/r/Azure/azure-cli/5952
This is an experimental preview for @microsoft users.

tjprescott

Huge step in the right direction!

tjprescott · 2018-03-29T20:50:25Z

azure-cli.pyproj

+      <PathEnvironmentVariable>
+      </PathEnvironmentVariable>
+      <Architecture>Amd64</Architecture>
+    </Interpreter>


Revert these changes.

tjprescott · 2018-03-29T22:26:03Z

tools/automation/cli_linter/__init__.py

+
+    print('Initializing linter with command table and help files...')
+    # setup CLI to enable command loader
+    az_cli = TestCli()


Why TestCli instead of the regular CLI?

Good question

tjprescott · 2018-03-29T22:26:44Z

tools/automation/cli_linter/__init__.py

+    create_invoker_and_load_cmds_and_args(az_cli)
+    loaded_help = get_all_help(az_cli)
+    command_table = az_cli.invocation.commands_loader.command_table
+    add_id_parameters(None, cmd_tbl=command_table)


Shouldn't create_invoke_and_load_cmds_and_args do this?

It doesn't...though maybe it should

Yes, it should...

I mean, it is really loading the commands and args if it leaves args out?

tjprescott · 2018-03-29T22:31:39Z

tools/automation/cli_linter/linter.py

+        if rule_type in self._rules:
+            self._rules.get(rule_type).append(rule_callable)
+
+    def mark_rule_failure(self):


Why public?

Because this needs to be exposed to the rules themselves. Each rule is responsible for alerting the linter if it fails.

tjprescott · 2018-03-29T22:31:47Z

tools/automation/cli_linter/linter.py

+    def get_parameter_exclusions(self, command_name):
+        return self._exclusions.get('params').get(command_name, {})
+
+    def add_rule(self, rule_type, rule_callable):


Why public?

tjprescott · 2018-03-29T23:02:21Z

tools/automation/cli_linter/rules/command_rules.py

+
+@command_rule('Checking for --ids parameter in list commands...')
+def no_ids_for_list_commands_rule(linter, command_name):
+    if command_name.split()[-1] == 'list' and 'ids' in linter.get_command_parameters(command_name):


I would expect command_name here to be "list" automatically and not have to do this split. Consider having linter, command_name and command_path for these rule types.

The command_name (as a string) is used as keys to access lots of different information in the linter and will likely be used by almost every rule.
The command_path is not as useful except for identifying a particular aspect of a command (i.e. checking for list or update commands) which seems more of a situational use. I would also like to avoid duplication in the arguments given to the rules.

tjprescott · 2018-03-29T23:31:57Z

tools/automation/cli_linter/rules/exclusions.json

+{
+    "help_entries": {
+        "example_entry": [
+            "unrecognized_help_entry_rule"


I think we should definitely reconsider this. A single dumping ground for all exceptions would, I think, grow unwieldy. I think, as Derek suggested, having an exception file per module/extension is the better way to go.

Hmm, that wouldn't be applicable to the help_entry rules and it seems like a lot of extra files that may only have one rule exception in them. It would be the same information just scattered throughout the modules.

Having one source gives the benefit of making it very easy to quickly search for and add new exclusions. Maybe we can save this discussion when we have more exclusions and see which is more usable?

This piece should be vetted through the whole team, but I see some of your points.

I would also add that JSON is a pretty unfriendly format and would recommend something else (YAML, config format, etc.)

tjprescott · 2018-03-29T23:34:19Z

tools/automation/cli_linter/rules/help_rules.py

+
+@help_file_entry_rule('Checking unrecognized commands and command-groups in help...')
+def unrecognized_help_entry_rule(linter, help_entry):
+    if help_entry not in linter.get_commands().union(linter.get_command_groups()):


Since help entry rules would often be concerned with this "command or group" scenario, perhaps it would make sense to precompile this list in the linter so you don't have to keep creating this union, testing it, and throwing it away. Maybe something like:

def find_command_or_group_help(self, name): """ Return the help entry for the specified command or command group, or None if not found. """ return self._master_help.get(name, None)

I'll do two condition checks to avoid this. I don't foresee many rules needing access to both.
Thanks

tjprescott · 2018-03-29T23:41:16Z

tools/automation/cli_linter/rules/help_rules.py

+def faulty_help_type_rule(linter, help_entry):
+    mark = True
+    if linter.get_help_entry_type(help_entry) != 'group' and help_entry in linter.get_command_groups():
+        print('--Help-Entry: `%s`- Command-group should be help-type `group`.' % help_entry)


Why don't you just mark failure here?

tjprescott · 2018-03-29T23:41:23Z

tools/automation/cli_linter/rules/help_rules.py

+    if linter.get_help_entry_type(help_entry) != 'group' and help_entry in linter.get_command_groups():
+        print('--Help-Entry: `%s`- Command-group should be help-type `group`.' % help_entry)
+    elif linter.get_help_entry_type(help_entry) != 'command' and help_entry in linter.get_commands():
+        print('--Help-Entry: `%s`- Found in command table but is not of help-type `command`.' % help_entry)


And here? If the failure and message were combined, this would be a lot more succinct.

derekbekoe · 2018-03-30T18:54:23Z

src/azure-cli-core/azure/cli/core/util.py

+
+def _is_group(parser):
+    return getattr(parser, '_subparsers', None) is not None \
+        or getattr(parser, 'choices', None) is not None


Would prefer this change in a separate file like help_util.py because none of this is actually used by the CLI product yet other methods in here are used by the CLI in actual command usage.

I can do that.
I'll make interactive leverage this soon so that it is used by other areas in the CLI.

It would be good to connect with the VSCode guys as well. They need some public functions like this to insulate their extension form implementation changes within the CLI that cause their stuff to break.

derekbekoe · 2018-03-30T18:55:21Z

src/command_modules/azure-cli-interactive/HISTORY.rst

@@ -3,6 +3,10 @@
 Release History
 ===============

+0.3.19
++++++
+* Add id parameters to be used by completers.


What does this mean?
Can you add more info of make it clearer?

I'll change this to "Allow completions for --ids parameters"

derekbekoe · 2018-03-30T18:57:03Z

tools/automation/cli_linter/linter.py

+from pkgutil import iter_modules
+
+
+class Linter():


Classes should inherit from object
https://stackoverflow.com/questions/4015417/python-class-inherits-object

It may be worth trying the linter on Python 2 locally to see if it works there also.
Since the CLI supports py2 and 3, our dev tools should also ideally.

works in python2 :)

derekbekoe · 2018-03-30T19:00:21Z

tools/automation/cli_linter/linter.py

+        self._exit_code = 1
+
+    def get_exit_code(self):
+        return self._exit_code


In Python, you don't need all these get_X(self) methods.
You can do this:

@property def exit_code(self): return self._exit_code

At least this is fine for the ones that only have self as an argument.

derekbekoe · 2018-03-30T19:03:00Z

tools/automation/cli_linter/linter.py

+    def run(self, run_params=None, run_commands=None, run_command_groups=None, run_help_files_entries=None):
+        paths = import_module('automation.cli_linter.rules').__path__
+        exclusion_path = os.path.join(paths[0], 'exclusions.json')
+        self._exclusions = json.load(open(exclusion_path))


If there is no exclusions file, does this still work?

Definitely not, I can make it so it does.

derekbekoe · 2018-03-30T19:11:02Z

tools/automation/cli_linter/rules/command_group_rules.py

+@command_group_rule('Checking missing help for command-groups...')
+def missing_group_help_rule(linter, command_group_name):
+    if not linter.get_command_group_help(command_group_name):
+        print('--Command-Group: `%s`- Missing help.' % command_group_name)


I'm not convinced of the linter.mark_rule_failure() pattern.
Python has Exceptions so we should leverage that.
For example, default a base class LinterException and allow others to subclass this exception type.
Then at some central place, you have a try/ except LinterException then let the exception print whatever details it needs and when you get at least one LinterException, you know to exit with code 1.

tjprescott · 2018-03-30T21:33:01Z

.travis.yml

@@ -55,3 +59,5 @@ jobs:
      python: 3.6
      env: PURPOSE='Automation Docker'
      if: repo = Azure/azure-cli and type = push
+  allow_failures:
+    - env: PURPOSE='Lint Command Table and Help'


It's questionable why we whould really want a build stage that allows failures. I think historically, these get totally ignored.

So can this go away now?

tjprescott · 2018-03-30T21:34:53Z

src/azure-cli-core/azure/cli/core/util.py

+
+def _is_group(parser):
+    return getattr(parser, '_subparsers', None) is not None \
+        or getattr(parser, 'choices', None) is not None


It would be good to connect with the VSCode guys as well. They need some public functions like this to insulate their extension form implementation changes within the CLI that cause their stuff to break.

tjprescott · 2018-03-30T21:35:46Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_help.py

@@ -197,7 +197,7 @@
 """

 helps['vm availability-set delete'] = """
-    type: command'
+    type: command


Look at the linter already doing its job!!!

tjprescott · 2018-03-30T21:36:14Z

tools/automation/cli_linter/__init__.py

+    create_invoker_and_load_cmds_and_args(az_cli)
+    loaded_help = get_all_help(az_cli)
+    command_table = az_cli.invocation.commands_loader.command_table
+    add_id_parameters(None, cmd_tbl=command_table)


Yes, it should...

I mean, it is really loading the commands and args if it leaves args out?

tjprescott · 2018-03-30T21:40:20Z

tools/automation/cli_linter/rules/command_group_rules.py

+@command_group_rule('Checking missing help for command-groups...')
+def missing_group_help_rule(linter, command_group_name):
+    if not linter.get_command_group_help(command_group_name):
+        print('--Command-Group: `%s`- Missing help.' % command_group_name)


I think you need to distinguish between a linter violation, exception, and warning. For example, the conditional logic in the callable should concern itself only with whether a linter violation occurred (i.e. is this the situation I was designed to detect?). If so, the rule should have metadata of some kind to say whether a violation should be interpreted as an Error (exit code 1) or a Warning (exit code 0)

tjprescott · 2018-03-30T21:41:52Z

tools/automation/cli_linter/rules/exclusions.json

+{
+    "help_entries": {
+        "example_entry": [
+            "unrecognized_help_entry_rule"


This piece should be vetted through the whole team, but I see some of your points.

I would also add that JSON is a pretty unfriendly format and would recommend something else (YAML, config format, etc.)

linter manager, some feedback

--modules, yml exclusions transferred to __init__.py

tjprescott

My only request is to remove the "allow_failures" from the CI and limit the run to those tests that are passing.

tjprescott · 2018-04-04T22:43:08Z

.travis.yml

@@ -55,3 +59,5 @@ jobs:
      python: 3.6
      env: PURPOSE='Automation Docker'
      if: repo = Azure/azure-cli and type = push
+  allow_failures:
+    - env: PURPOSE='Lint Command Table and Help'


So can this go away now?

derekbekoe · 2018-04-04T23:59:55Z

src/command_modules/azure-cli-vm/linter_exclusions.yml

+            rule_exclusions:
+            - missing_parameter_help_rule
+
+...


Like the new format!
What's the ... for at the end?

Indicates the end of a file.
It's optional, similar to the --- at the start.

derekbekoe · 2018-04-05T00:03:16Z

tools/automation/cli_linter/rules/linter_exclusions.yml

+---
+# this is an example linter exclusion file
+
+help-entry-name:


Consider adding a comment of an example.
help-entry-name still doesn't tell me what to actually type.
Would it be what is in helps['this text'] as this text?

yep, that's exactly it

williexu requested review from derekbekoe and troydai March 29, 2018 19:39

williexu assigned tjprescott Mar 29, 2018

williexu force-pushed the commandlint branch 5 times, most recently from 493e557 to eb1eb50 Compare March 29, 2018 20:58

williexu requested review from tjprescott and yugangw-msft March 29, 2018 23:01

tjprescott suggested changes Mar 29, 2018

View reviewed changes

derekbekoe suggested changes Mar 30, 2018

View reviewed changes

tjprescott reviewed Mar 30, 2018

View reviewed changes

williexu force-pushed the commandlint branch 3 times, most recently from 77b0cca to 7ad9324 Compare April 4, 2018 18:38

williexu added 10 commits April 4, 2018 11:39

basic command linter with a few rules

d1c341f

added missing file

7f34278

added group help check rule

e20e5e8

adding doc help gen to core util

1c3e39d

exclusions, exit code, parameter rules, leverage doc helpgen

c57132a

add linter to ci

30381c3

linter manager, some feedback

allow failure and revert pyproj

87b6b1b

ci mode, combined print/failing code, other small fixes

55d5a77

sort violations and remove starting messages

14eea4e

added module scoping for linter

d84ecba

--modules, yml exclusions transferred to __init__.py

williexu force-pushed the commandlint branch from 7ad9324 to d84ecba Compare April 4, 2018 18:39

tjprescott suggested changes Apr 4, 2018

View reviewed changes

tjprescott approved these changes Apr 4, 2018

View reviewed changes

derekbekoe approved these changes Apr 5, 2018

View reviewed changes

williexu force-pushed the commandlint branch from e4a4bd9 to d84ecba Compare April 5, 2018 08:28

some exclusions, linter fails ci

1dfad31

tjprescott merged commit 48a0c83 into Azure:dev Apr 5, 2018

williexu deleted the commandlint branch July 11, 2018 17:27

Command linter #5952

Command linter #5952

Conversation

williexu commented Mar 29, 2018 • edited Loading

promptws commented Mar 29, 2018

tjprescott left a 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

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

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

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

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williexu Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williexu commented Mar 29, 2018 •

edited

Loading

williexu Apr 5, 2018 •

edited

Loading