Skip to content

[ARM]Support policy exemption#17565

Merged
zhoxing-ms merged 13 commits intoAzure:devfrom
robga:robga/policy
Apr 9, 2021
Merged

[ARM]Support policy exemption#17565
zhoxing-ms merged 13 commits intoAzure:devfrom
robga:robga/policy

Conversation

@robga
Copy link
Member

@robga robga commented Apr 5, 2021

Description

  1. Upgrade azure-mgmt-resource SDK version to 12.1.0
  2. Add policy exemption commands
  3. Add unit test for policy exemption commands
  4. "sku" property is removed from Policy assignment properties in the SDK.
  5. Since the SDK version is increased, re-recorded some tests

Testing Guide

History Notes


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

@yonzhan yonzhan added this to the S185 milestone Apr 5, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 5, 2021

ARM

@robga
Copy link
Member Author

robga commented Apr 5, 2021

@zhoxing-ms
The linter threw an error.
FAIL - HIGH severity: faulty_help_example_rule
Help-Entry: policy exemption update - The following example entry indices do not include the command: 1 | 2
I didn't follow what that means. Could you please point it out what I should change?

@robga
Copy link
Member Author

robga commented Apr 6, 2021

@zhoxing-ms , for the only test failure left.
FAILED src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py::SqlManagedDatabaseLogReplayScenarionTest::test_sql_midb_logreplay_mgmt
The test seems is setup for the owner's environment. The policy assignment name is hardcoded. I don't have their environment to record the test again. Please advice.

@robga
Copy link
Member Author

robga commented Apr 6, 2021

@zhoxing-ms , I should remove the 'sku' parameter for policy assignment. Is that a breaking change? How should I remove it without breaking any existing usage?
It's line 190 in the file (src/azure-cli/azure/cli/command_modules/resource/_params.py )
c.argument('sku', options_list=['--sku', '-s'], help='policy sku.', arg_type=get_enum_type(['free', 'standard']))

@zhoxing-ms
Copy link
Contributor

@robga Hi, since we plan to complete the migration of track 2 for resource module this month, we will upgrade the SDK version of azure-mgmt-resource to 16.0.0 (policy exemption is also included in this SDK version)
Because this PR will have some conflicts with the migration of track 2, so may I ask what is the expected release time for this requirement? (If it is not an urgent requirement, maybe we can consider merging this PR after the migration of track 2.)

@robga
Copy link
Member Author

robga commented Apr 6, 2021

Hi, @zhoxing-ms , I want to merged it in ASAP. I hope to catch the 4/7 deadline for this PR.
I will really appreciate if you can let my PR merge first. I think you probably much more familiar this project than I do.
The merge could take me much more time.

@zhoxing-ms
Copy link
Contributor

Hi, @zhoxing-ms , I want to merged it in ASAP. I hope to catch the 4/7 deadline for this PR.
I will really appreciate if you can let my PR merge first. I think you probably much more familiar this project than I do.
The merge could take me much more time.

OK, we will release the PR in this sprint (04/13/2021)

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Apr 6, 2021

for the only test failure left.
FAILED src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py::SqlManagedDatabaseLogReplayScenarionTest::test_sql_midb_logreplay_mgmt
The test seems is setup for the owner's environment. The policy assignment name is hardcoded. I don't have their environment to record the test again. Please advice.

The following error occurred when executing command az policy assignment show -n {policy_name} in the test in the live mode:

msrestazure.azure_exceptions.CloudError: Azure Error: PolicyAssignmentNotFound
Message: The policy assignment 'SDOStdPolicyNetwork' is not found.

Whether this test needs to add creation logic for policy assignment before this command or use other solutions, @Juliehzl please help to have a look and give some advice~

@zhoxing-ms
Copy link
Contributor

I should remove the 'sku' parameter for policy assignment. Is that a breaking change? How should I remove it without breaking any existing usage?
It's line 190 in the file (src/azure-cli/azure/cli/command_modules/resource/_params.py )
c.argument('sku', options_list=['--sku', '-s'], help='policy sku.', arg_type=get_enum_type(['free', 'standard']))

Yes, it's a breaking change, because deleting parameter sku directly may affect the user's existing script.

You can add an deprecate annotation for this parameter to let the user know in advance that this parameter will be deprecated.
When no user uses this parameter any more (user usage can be evaluated according to Telemetry), then remove the parameter completely.
Please refer to document deprecating-commands-and-arguments for more details on deprecate parameters

@robga robga requested a review from jaredmoo as a code owner April 7, 2021 16:56
Copy link
Contributor

@zhoxing-ms zhoxing-ms left a comment

Choose a reason for hiding this comment

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

Because there are some nonstandard writing style in the old code that have been referenced in this PR, I approve this PR at first. We could consider refactoring the overall nonstandard part in the future.

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

I still have concern for code style. But as it is close to build, approved to merge the feature. But please refactor the code in future. Next time we will not approve for non-standard code style.

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