Skip to content

Conversation

@fengzhou-msft
Copy link
Member

@fengzhou-msft fengzhou-msft commented Jul 23, 2020

Description

When a command failed in parser._check_value due to a command word is not in the choice list of its previous word, such as az vm abc would fail because abc is not a choice under vm. This failure could be caused by:

  1. Typo, e.g. az accuont show or use get instead of the correct show.
  2. The command is from an extension but the extension is not installed.

Previously, we cannot tell the difference and the error message is like this:

az: 'bluprint' is not in the 'az' command group. See 'az --help'. If the command is from an extension, please make sure the corresponding extension is installed. To learn more about extensions, please visit https://docs.microsoft.com/en-us/cli/azure/azure-cli-extensions-overview

This PR adds a search for the unmatched command in all the extension commands if a close match for the wrong command is not found in all module commands (indicating it's not likely a typo). If the command (not including options) matches a complete extension command, based on config, we can automatically install the extension with or without prompt. Then the command will continue to run by default or users can choose to rerun by themselves.

A mapping of all extension commands to extension names will be fetched from a storage account and stored in ~/.azure/extCmdIndex.json. It is valid for 10 days and will only be refreshed when a parse error happens. An example of the file:

{
       "aks": {
                "create": "aks-preview",
                "update": "aks-preview",
                "app": {
                    "up": "deploy-to-azure"
                },
                "use-dev-spaces": "dev-spaces"
        },
        ...
}

Configuration:
Both config options are under extension section:

  1. use_dynamic_install
    yes_prompt: it will use dynamic extension install and prompt to user whether they would like to install the needed extension.
    yes_without_prompt: it will install the needed extension directly without confirmation from users.
    no: it falls back to previous behavior and just throws an error. This is the default for now.

  2. run_after_dynamic_install
    yes/true: It’s the default and will continue to run the command after extension is installed.
    no/false: It will stop when extension is installed and ask the user to rerun the command.

You can also set with environment variables (AZURE_EXTENSION_USE_DYNAMIC_INSTALL, AZURE_EXTENSION_RUN_AFTER_DYNAMIC_INSTALL)and they have higher priority.

Testing Guide

1.set env var to turn on the feature:
export AZURE_EXTENSION_USE_DYNAMIC_INSTALL=yes_promt
2.run a command from an extension not installed to chekc the behavior.
3.run a real error command to chekc the new clear error message.


This checklist is used to make sure that common guidelines for a pull request are followed.

@fengzhou-msft fengzhou-msft marked this pull request as ready for review July 23, 2020 09:33
@yonzhan
Copy link
Collaborator

yonzhan commented Jul 23, 2020

Extension

@fengzhou-msft fengzhou-msft requested a review from houk-ms July 29, 2020 10:57
Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add test for this change ?

cmd_list = self.prog.split() + self._raw_arguments
command_str = roughly_parse_command(cmd_list[1:])
ext_name = self._search_in_extension_commands(command_str)
if ext_name:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if ext_name is None ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ext_name is None, error_msg will be None and below code will use the {prog}: '{value}' is not in the '{prog}' command group. for error message.

@fengzhou-msft
Copy link
Member Author

Is it possible to add test for this change ?

Test added.

Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return None

def _get_extension_use_dynamic_install_config(self):
cli_ctx = self.cli_ctx or (self.cli_help.cli_ctx if self.cli_help else None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cli_ctx seems could be None and then line 342 would crash.

Copy link
Member Author

@fengzhou-msft fengzhou-msft Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 342 has the logic if cli_ctx else 'no' to handle None, it will use 'no' as the value.

verify=(not should_disable_connection_verify()),
timeout=300)
except Exception as ex: # pylint: disable=broad-except
logger.info("Request failed for extension command tree: %s", str(ex))
Copy link
Contributor

@haroldrandom haroldrandom Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would log at warning level be better?

Copy link
Member Author

@fengzhou-msft fengzhou-msft Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, let's just fall back to previous behavior when something is wrong as the warning may confuse users when they only typed a wrong command.

EXT_CMD_TREE.data = response.json()
EXT_CMD_TREE.save_with_retry()
else:
logger.info("Error when retrieving extension command tree. Response code: %s", response.status_code)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would log at warning level be better?

@fengzhou-msft fengzhou-msft merged commit 04a692a into Azure:dev Jul 31, 2020
@fengzhou-msft fengzhou-msft deleted the dynamic_extension_install branch July 31, 2020 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants