[Core] enable local context for global resource group #12741
Conversation
arrownj
requested review from
bim-msft,
jiasli,
qwordy,
jsntcy,
qianwens,
yungezz,
mmyyrroonn and
fengzhou-msft
arrownj
requested review from
haroldrandom,
Juliehzl and
zhoxing-ms
as code owners
src/azure-cli/azure/cli/command_modules/resource/tests/latest/test_local_context.py
Show resolved
Hide resolved
|
local context |
|
It'll be great if the PR description can have some brief introduction about the implementation:
The design spec is user-oriented. We'd better also have some developer-orientated doc. |
src/azure-cli/azure/cli/command_modules/resource/tests/latest/test_local_context.py
Show resolved
Hide resolved
| dir_path = os.path.join(current_dir, dir_name) | ||
| file_path = os.path.join(dir_path, file_name) | ||
| if os.path.isfile(file_path): | ||
| self._file_chain.append(_ConfigFile(dir_path, file_path)) |
There was a problem hiding this comment.
use os.access(path, mode) to check whether have R/W permission.
| help='name of the new resource group', completer=None, lc_actions=[STORE], lc_scopes=[GLOBAL], | ||
| lc_name='resource_group_name') |
There was a problem hiding this comment.
Let's extract lc_actions, lc_scopes and lc_name to a LocalContext class, like deprecate_info
def deprecate(self, **kwargs):
kwargs['object_type'] = 'command'
return Deprecated(self.command_loader.cli_ctx, **kwargs)There was a problem hiding this comment.
I used a LocalContextAttribute class and it is a little bit different with Deprecated. Please take a look at it.
| STORE = 'STORE' # action for a parameter in local context, STORE means its value will be saved to local context | ||
| USE = 'USE' # action for a parameter in local context, USE means will read value from local context for this parameter | ||
| ALL = 'ALL' # effective level of local context, ALL means all commands can share this parameter value |
There was a problem hiding this comment.
| STORE = 'STORE' # action for a parameter in local context, STORE means its value will be saved to local context | |
| USE = 'USE' # action for a parameter in local context, USE means will read value from local context for this parameter | |
| ALL = 'ALL' # effective level of local context, ALL means all commands can share this parameter value | |
| STORE = 'STORE' # action for a parameter in local context. STORE means its value will be saved to local context | |
| USE = 'USE' # action for a parameter in local context. USE means the value of this parameter will be read from local context | |
| ALL = 'ALL' # effective level of local context. ALL means all commands can share this parameter value |
| """ Local Context Attribute arguments | ||
|
|
||
| Save argument value to local context if it is defined as STORE and user specify a value for it. | ||
|
|
||
| :param parsed_args: Parsed args which return by AzCliCommandParser parse_args | ||
| :type parsed_args: Namespace | ||
| :param argument_definitions: All available argument definitions | ||
| :type argument_definitions: dict | ||
| :param specified_arguments: Arguments which user specify in this command | ||
| :type specified_arguments: list | ||
| """ |
There was a problem hiding this comment.
| """ Local Context Attribute arguments | |
| Save argument value to local context if it is defined as STORE and user specify a value for it. | |
| :param parsed_args: Parsed args which return by AzCliCommandParser parse_args | |
| :type parsed_args: Namespace | |
| :param argument_definitions: All available argument definitions | |
| :type argument_definitions: dict | |
| :param specified_arguments: Arguments which user specify in this command | |
| :type specified_arguments: list | |
| """ | |
| """ Local Context Attribute arguments | |
| Save argument value to local context if it is defined as STORE and user specifies a value for it. | |
| :param parsed_args: Parsed args returned by AzCliCommandParser parse_args | |
| :type parsed_args: Namespace | |
| :param argument_definitions: All available argument definitions | |
| :type argument_definitions: dict | |
| :param specified_arguments: Arguments which user specifies in this command | |
| :type specified_arguments: list | |
| """ |
| @@ -9,7 +9,7 @@ | |||
|
|
|||
| CLI_COMMON_KWARGS = ['min_api', 'max_api', 'resource_type', 'operation_group', | |||
| 'custom_command_type', 'command_type', 'is_preview', 'preview_info', | |||
| 'is_experimental', 'experimental_info'] | |||
| 'is_experimental', 'experimental_info', 'local_context_attribute'] | |||
There was a problem hiding this comment.
as we synced offline, is it really necessary to add the attribute chaneges in this release? this release suppose be a light change only for resource group by extending existing az config.
| self._load_file_chain() | ||
| except Exception: # pylint: disable=broad-except | ||
| raise CLIError('fail to turn on local context in {}.'.format(os.path.dirname(current_dir))) | ||
| logger.warning('local context in current directory is turned on.') |
There was a problem hiding this comment.
consistent message, either enable or turned on
Co-Authored-By: Jiashuo Li <[email protected]>
| if value: | ||
| logger.debug("local context '%s' for arg %s", value, arg.name) | ||
| overrides.settings['default'] = DefaultStr(value) | ||
| overrides.settings['required'] = False |
There was a problem hiding this comment.
overrides.settings['required'] = False [](start = 20, length = 38)
will this statement "overrides.settings['required'] = False" impact the ux like -h ?
There was a problem hiding this comment.
Good point. Yes, if the value is saved in local context, it will show the value as default in -h. I think we need add more information for this, such as from local context.
And if local context is not turned on, -h will behavior as before.
| def _get_values(self, action, arg_strings): | ||
| value = super(AzCliCommandParser, self)._get_values(action, arg_strings) | ||
| if action.dest and isinstance(action.dest, str) and not action.dest.startswith('_'): | ||
| self.specified_arguments.append(action.dest) |
There was a problem hiding this comment.
looks tricky to add this logic in a _get_value method
There was a problem hiding this comment.
this is a function which is called in the ArgumentParser.parse_args(). The input for parse_args() is the command string(such as vm create -g rg -n vm_name), and _get_values() is called per parameter. For the given example, it will call _get_values for -g and -n. And here we want to collect the user specified arguments, I think it is a good place to add logic here.
| # Stop if already in root drive | ||
| if current_dir == os.path.dirname(current_dir): | ||
| break | ||
| current_dir = os.path.dirname(current_dir) |
There was a problem hiding this comment.
if cwd is d:\c1, you can only have two sessions to use context file? I think it is not a good idea to add new file in different folder levels. can you use different file name if the file is locked by other session?
There was a problem hiding this comment.
| raise CLIError('fail to turn on local context in {}.'.format(current_dir)) | ||
| logger.warning('local context in %s is turned on.', current_dir) | ||
| else: | ||
| raise CLIError('local context is already turned on in {}'.format(current_dir)) |
There was a problem hiding this comment.
raise CLIError [](start = 12, length = 14)
I think it is better to just logger.warning here, no need to raise error if it is already on
| break # load only one local context | ||
| # Stop if already in root drive | ||
| if current_dir == os.path.dirname(current_dir): | ||
| break |
There was a problem hiding this comment.
break [](start = 16, length = 5)
raise error when turn_on ?
| g.command('on', 'turn_local_context_on') | ||
| g.command('off', 'turn_local_context_off') |
There was a problem hiding this comment.
The command name comes from PM? According to command name convention, the last word should be a verb. I'm personally fine with the on/off, not sure if we have simliar existing commands.
There was a problem hiding this comment.
yes, PM prefer such commands ...
| # Stop if already in root drive | ||
| if current_dir == os.path.dirname(current_dir): | ||
| if self._local_context_file is None: | ||
| logger.debug('local context is not turned on in %s and all its parent directories', current_dir) |
There was a problem hiding this comment.
current_dir is now root, should it be the original directory get by os.getcwd()?
There was a problem hiding this comment.
this is just debug information, for simplicity, let's just keep this message.
| @@ -8,6 +8,7 @@ | |||
| import unittest | |||
|
|
|||
| from azure.cli.testsdk import ScenarioTest | |||
| from knack.util import CLIError | |||
There was a problem hiding this comment.
remove this unused import.
This is the first version for local context.
You can find the very early spec here.
There are also some other useful linkes:
And we have come with an agreement with PM that we will divide these works into several phases.
In this sprint, we add two new commands:
and only enable the global parameter
--resource-group-name.After this change, if user run a command and specify
--resource-group-namefor it, the value will automatically saved and will be used in following commands if the local-context is turned on.Description of PR (Mandatory)
(Why this PR? What is changed? What is the effect? etc. A high-quality description can accelerate the review process)
Testing Guide
(Example commands with explanations)
History Notes:
(Fill in the following template if multiple notes are needed, otherwise PR title will be used for history note.)
[Component Name 1] (BREAKING CHANGE:) (az command:) make some customer-facing change.
[Component Name 2] (BREAKING CHANGE:) (az command:) make some customer-facing change.
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.