-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[ARM] az group export: Add new parameters --skip-resource-name-params and --skip-all-params to support skip parameterization
#13558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
add to S170 |
|
LGTM! |
|
@jorgecotillo Hi, I suggest that it might be better to change the PR title, like: Please refer to the document when writing PR title: doc link |
|
Testing Guide
This command should actually be |
.vscode/launch.json
Outdated
| "type": "python", | ||
| "request": "launch", | ||
| "pythonPath": "${config:python.pythonPath}", | ||
| "pythonPath": "${config:python.interpreterPath}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it modified here?
|
|
||
| def export_group_as_template( | ||
| cmd, resource_group_name, include_comments=False, include_parameter_default_value=False): | ||
| cmd, resource_group_name, resource_ids=None, include_comments=False, include_parameter_default_value=False, skip_resource_name_parameterization=False, skip_all_parameterization=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resource_ids=None It is recommended to put the new parameters after the existing parameter of the method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--skip-all-parameterization and --skip-resource-name-parameterization are too long for the customers to input. Could you make them shorter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about --skip-all-params and --skip-resource-name-params? @jorgecotillo
| raise CLIError('az resource: error: argument --resource_ids: invalid ResourceId value: \'%s\'' % i) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--resource_ids --> --resource-ids
|
|
||
| self.cmd('network vnet create -g {rg} -n {vnet}') | ||
| self.kwargs['vnet_id'] = self.cmd('network vnet show -g {rg} -n {vnet}').get_output_in_json()['id'] | ||
| result = self.cmd('group export --name {rg} --resource-ids "{vnet_id}" --skip-all-parameterization --query "contentVersion"') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add checks to this test to confirm whether the --skip-all-parameterization is effective?
|
|
||
| self.cmd('network vnet create -g {rg} -n {vnet}') | ||
| self.kwargs['vnet_id'] = self.cmd('network vnet show -g {rg} -n {vnet}').get_output_in_json()['id'] | ||
| result = self.cmd('group export --name {rg} --resource-ids "{vnet_id}" --skip-resource-name-parameterization --query "contentVersion"') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add checks to this test to confirm whether the --skip-resource-name-parameterization is effective?
--skip-resource-name-parameterization and --skip-all-parameterization to support skip parameterization
--skip-resource-name-parameterization and --skip-all-parameterization to support skip parameterization--skip-resource-name-params and --skip-all-params to support skip parameterization
|
Can someone give me a hint on how to solve the following issue: AssertionError: No match for the request (<Request (PUT) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cli_test_rg_scenario000001/providers/Microsoft.Network/virtualNetworks/vnet1?api-version=2020-04-01>) was found. Can't overwrite existing cassette ('/home/vsts/work/1/s/src/azure-cli/azure/cli/command_modules/resource/tests/latest/recordings/test_resource_group_export_skip_all_params.yaml') in your current record mode ('once'). When I run the test locally, I get vnet api-version 2020-03-01, I just merged the latest from master and still seeing the same api-version and an additional error: test_resource_scenario is also complaining. Can I get some assistance please? |
@jorgecotillo Hi, this problem is because the version of |
|
@zhoxing-ms merged from dev and now seeing different errors ... env/lib/python3.8/site-packages/_pytest/junitxml.py:417 -- Docs: https://docs.pytest.org/en/latest/warnings.html
|
|
@jorgecotillo This is a known issue: the problem is caused by an incompatibility with Python 3.8 in the self = <contextlib.ExitStack object at 0x7f4b609ab070>, exc_details = ()
def __exit__(self, *exc_details):
> received_exc = exc_details[0] is not None
E IndexError: tuple index out of rangeWe already have a PR follow-up to solve this problem: #13661 |
|
|
||
| def export_group_as_template( | ||
| cmd, resource_group_name, include_comments=False, include_parameter_default_value=False): | ||
| cmd, resource_group_name, include_comments=False, include_parameter_default_value=False, resource_ids=None, skip_resource_name_params=False, skip_all_params=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we register all new arguments in _params.py? Especially for --resource-ids, which should be an array, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, had to add nargs to the param.
| :param bool skip_resource_name_params: export template and skip resource name parameterization. | ||
| :param bool skip_all_params: export template parameter and skip all parameterization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these parameters, I will recommend you to add them as storage_true and give short name for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
|
add to S171 |
|
@Juliehzl can you please confirm the recent updates? Let me know if they look ok to you or if additional updates are needed. |
zhoxing-ms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@qianwens please let me know if the PR looks good or if updates are required. |
|
@qianwens please let us know if the PR is good to go or if there are any other updates we need to make. Thanks! |
| self.assertEqual('"1.0.0.0"\n', result.output) | ||
|
|
||
| @ResourceGroupPreparer(name_prefix='cli_test_rg_scenario') | ||
| def test_resource_group_export_skip_all_params(self, resource_group): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_resource_group_export_skip_all_params [](start = 8, length = 42)
please also add test case for parameter --resource-ids
| export_options.append('SkipAllParameterization') | ||
|
|
||
| resources = [] | ||
| if resource_ids is None or resource_ids == "*": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resource_ids [](start = 31, length = 12)
just to confirm here if it should be resource_ids[0] == "*"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the code.
|
@Juliehzl @qianwens @zhoxing-ms if the PR looks good, is it possible to merge it please? Otherwise, let me know if there are additional updates required. |
Description
--resource-ids argument to filter the results based on a string containing whitespace-separated
Testing Guide
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.