Add policy set definition commands #4515

vivsriaus · 2017-09-21T19:33:50Z

Add policy set definition CRUD commands
Add sku and policysetdefinition to policy assignment create command

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

General Guidelines

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test.

(see Authoring Command Modules)

lmazuel · 2017-09-22T18:02:40Z

@vivsriaus https://pypi.python.org/pypi/azure-mgmt-resource/1.2.0rc3 with your 404 fix

derekbekoe · 2017-09-22T18:35:15Z

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

+helps['policy setdefinition list'] = """
+    type: command
+    short-summary: List policy set definitions.
+"""


There is no description for az policy setdefinition -h

derekbekoe · 2017-09-22T18:36:33Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/_params.py

+                            help='policy set definition name or fully qualified id')
+    c.register_cli_argument('policy assignment create', 'sku', options_list=('--sku', '-s'),
+                            help='policy sku. One of Free or Standard.')
+


There is no help for --params of az policy setdefinition create and az policy setdefinition update

Will add, thanks

derekbekoe · 2017-09-22T18:39:43Z

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

@@ -189,6 +189,39 @@
    type: command
    short-summary: List policy definitions.
 """
+helps['policy setdefinition create'] = """


IMO setdefinition should be set-definition for all these new commands.

Or even just set unless there are "set" commands and "set-definition" commands...

set-definition sounds more apt. thanks

tjprescott · 2017-09-22T17:44:07Z

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

@@ -189,6 +189,39 @@
    type: command
    short-summary: List policy definitions.
 """
+helps['policy setdefinition create'] = """


You need group help text for the setdefinition command group.

tjprescott · 2017-09-22T17:46:28Z

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

+                    az policy setdefinition create -n readOnlyStorage --definitions \\
+                        [ \\
+                            { \\
+                                "policyDefinitionId": "/subscriptions/mySubId/providers/Microsoft.Authorization/policyDefinitions/storagePolicy" \\


Is a policy set just a collection of IDs, or is there more to it? If it is, it seems you could get away from the JSON object approach and just accepts a list of names and/or IDs to assemble the set definition.

Not just IDs, it can have the entire policy rule as json as well.

I'm sorry I misspoke above. The policy set cannot contain entire policy rule, but it can have a multiples of policyDefinitionId and parameters (which is json object), so either way, this is a complex object that requires file handling

tjprescott · 2017-09-22T17:48:56Z

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

+            parameters:
+                - name: --definitions
+                  type: string
+                  short-summary: Policy definitions in JSON format, or a path to a file containing JSON rules.


Recommendations: 1) don't accept JSON unless you truly have to. 2) If you do need this to be JSON, then just accept the JSON string. The help text would look like JSON string containing the policy definition. Use @{file} to load from a file. We are trying to move away from sniffing whether an input is JSON or a path through the @ symbol (which loads the contents of the file).

This should be a json, since it takes a json array which can have:

policy definition id or

policy rule

With regards to help, the definitions can also be a uri that points to a valid json file, just like the rules in policy definition

Since you are going for parity with policy definition create then disregard my comment. However, do make sure the help and behavior is consistent.

tjprescott · 2017-09-22T17:53:05Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/_params.py

+    c.register_cli_argument('policy assignment create', 'policysetdefinition', options_list=('--policy-set-definition', '-d'),
+                            help='policy set definition name or fully qualified id')
+    c.register_cli_argument('policy assignment create', 'sku', options_list=('--sku', '-s'),
+                            help='policy sku. One of Free or Standard.')


Instead of calling out the allowed values via help text, this should use the **enum_choice_list(...) approach to reflect on the SDK's enum type. If there is no enum type, you can give it a list ['Free', 'Standard']

tjprescott · 2017-09-22T17:53:59Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/_params.py

+                      help='JSON formatted string or a path to a file or uri with such content',
+                      type=file_type, completer=FilesCompleter())
+    c.register_cli_argument('policy assignment create', 'policysetdefinition', options_list=('--policy-set-definition', '-d'),
+                            help='policy set definition name or fully qualified id')


The common way of expressing this is "Name or ID of the policy set definition." Ditto for the --policy argument.

tjprescott · 2017-09-22T17:57:58Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+                             resource_group_name=None, scope=None, sku=None):
+    if policy and policysetdefinition:
+        raise CLIError('usage error: must specify only one --policy POLICY | \
+        --policy-set-definition POLICYSETDEFINITION')


recommend:
usage error: --policy NAME_OR_ID | --policy-set-definition NAME_OR_ID

Also, shouldn't you raise this error if the user supplies neither? You can use if bool(policy) == bool(policysetdefinition):

Good catch!

tjprescott · 2017-09-22T17:58:57Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

    policy_client = _resource_policy_client_factory()
    scope = _build_policy_scope(policy_client.config.subscription_id,
                                resource_group_name, scope)
-    policy_id = _resolve_policy_id(policy, policy_client)
+    policy_id = _resolve_policy_id(policy, policysetdefinition, policy_client)


recommend parameter name policy_set_definition or just policy_set for clarity.

tjprescott · 2017-09-22T18:00:14Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        if os.path.exists(definitions):
+            definitions = get_file_json(definitions)
+        else:
+            definitions = shell_safe_json_parse(definitions)


If you just accept JSON and point of the @{file} syntax, you can eliminate this source of potential bugs.

Reading your above comment I understand the reason to keep these commands similar, so this is fine.

tjprescott · 2017-09-22T18:40:14Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+        policy_definitions=definitions if definitions is not None else definition.policy_definitions,
+        description=description if description is not None else definition.description,
+        display_name=display_name if display_name is not None else definition.display_name,
+        parameters=params if params is not None else definition.parameters)


Recommend you change "params" to "parameters" for consistency with the group deployment commands. Additionally, just like group deployment create, and as recommended for the policy object, I'd recommend you simply make the argument take a JSON string, and call out the @{file} syntax to load from a file.

Since you are going for parity with the policy definition create command, this is fine.

…nsure no regression

derekbekoe · 2017-09-22T22:37:29Z

@vivsriaus I cancelled the CI run. I will restart CI after #4523 has been merged.

derekbekoe · 2017-09-22T23:01:52Z

Restarted.

…olicySet # Conflicts: # src/command_modules/azure-cli-vm/setup.py

vivsriaus · 2017-09-26T18:38:06Z

@derekbekoe @tjprescott @yugangw-msft Can you please review the latest changes? Thanks
cc: @cyl3392207

derekbekoe

Couple comments on use of \ but LGTM otherwise.

derekbekoe · 2017-09-26T18:55:00Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+    """
+    if bool(policy) == bool(policy_set_definition):
+        raise CLIError('usage error: --policy NAME_OR_ID | \
+        --policy-set-definition NAME_OR_ID')


nit: The formatting here is not quite right. When I run it, I see the following:

# az policy assignment create usage error: --policy NAME_OR_ID | --policy-set-definition NAME_OR_ID

You need to add a couple extra quotes to avoid the line-wrap from showing in the output.

raise CLIError('usage error: --policy NAME_OR_ID | '\ '--policy-set-definition NAME_OR_ID')

derekbekoe · 2017-09-26T18:56:54Z

src/command_modules/azure-cli-resource/azure/cli/command_modules/resource/custom.py

+                    kwargs_list.append(id_arg)
+                else:
+                    logger.error('az policy assignment create error: argument --not-scopes: \
+                    invalid notscopes value: \'%s\'' % id_arg)


Same here with the use of \.

Right now, you are doing:

'My-first-line-of-text\ My-second-line-of-text'

Rather, it should be:

'My-first-line-of-text'\ 'My-second-line-of-text'

…olicySet

…into AddPolicySet

derekbekoe · 2017-09-26T21:17:37Z

@vivsriaus CI is failing due to style.
You can run check_style --ci to check locally.

vivsriaus · 2017-09-26T21:25:03Z

@derekbekoe Thanks, done.

Initial commit for policy set definition commands

df4c539

msftclas added the cla-not-required label Sep 21, 2017

vivsriaus added 2 commits September 21, 2017 14:29

Add sku, policysetdefinition to policy assignment create

69ec6d6

move policy set definition under api check

98c4048

derekbekoe closed this Sep 22, 2017

derekbekoe reopened this Sep 22, 2017

msftclas added the cla-not-required label Sep 22, 2017

derekbekoe suggested changes Sep 22, 2017

View reviewed changes

tjprescott suggested changes Sep 22, 2017

View reviewed changes

vivsriaus added 5 commits September 22, 2017 12:50

Address some feedback

cf9fe45

Add notScopes to assignment create, new record for existing test to e…

9ba706e

…nsure no regression

Fix not scopes issues

e78019c

fix pylint issue

9988cc6

Check for free sku in policy assignment

613f40b

vivsriaus added 7 commits September 22, 2017 16:27

Add checks for not scopes

bea8810

Merge branch 'master' of https://github.com/Azure/azure-cli into AddP…

26b629d

…olicySet # Conflicts: # src/command_modules/azure-cli-vm/setup.py

Add policy set tests

25188b8

Fix policy record

878fe46

Fix pylint

0af53b1

nits

dfc1e15

Fix CI issue

588b86f

msftgits removed the cla-not-required label Sep 26, 2017

derekbekoe suggested changes Sep 26, 2017

View reviewed changes

vivsriaus added 3 commits September 26, 2017 13:29

Merge branch 'master' of https://github.com/Azure/azure-cli into AddP…

c510185

…olicySet

Merge branch 'AddPolicySet' of https://github.com/vivsriaus/azure-cli …

52c9e91

…into AddPolicySet

Fix error message format

b44f552

derekbekoe approved these changes Sep 26, 2017

View reviewed changes

tjprescott approved these changes Sep 26, 2017

View reviewed changes

fix check style issue

820856d

derekbekoe merged commit 9b6260c into Azure:master Sep 26, 2017

Add policy set definition commands #4515

Add policy set definition commands #4515

Conversation

vivsriaus commented Sep 21, 2017 • edited Loading

General Guidelines

Command Guidelines

lmazuel commented Sep 22, 2017

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 Sep 22, 2017 • edited Loading

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

derekbekoe commented Sep 22, 2017

derekbekoe commented Sep 22, 2017

vivsriaus commented Sep 26, 2017

derekbekoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbekoe commented Sep 26, 2017

vivsriaus commented Sep 26, 2017

vivsriaus commented Sep 21, 2017 •

edited

Loading

tjprescott Sep 22, 2017 •

edited

Loading