Command module for Microsoft.PolicyInsights #6378

bulentelmaci · 2018-05-17T19:07:32Z

Command module for the new Microsoft.PolicyInsights RP.

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

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).
I adhere to the Command Guidelines.

…ams, help, completers, etc.)

…icyinsights" command group)

promptws · 2018-05-17T19:07:34Z

View a preview at https://prompt.ws/r/Azure/azure-cli/6378
This is an experimental preview for @microsoft users.

codecov-io · 2018-05-17T20:47:27Z

Codecov Report

Merging #6378 into dev will not change coverage.
The diff coverage is n/a.

@@         Coverage Diff         @@
##           dev   #6378   +/-   ##
===================================
  Coverage    0%      0%           
===================================
  Files       11      11           
  Lines      133     133           
  Branches     9       9           
===================================
  Misses     133     133

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee4bb56...dd1fa77. Read the comment docs.

tjprescott · 2018-05-17T21:51:32Z

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

+          type: string
+
+        - name: --apply
+          type: string


None of these parameters entries are required.

thanks. will remove

tjprescott · 2018-05-17T21:52:01Z

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

+        - name: --policy-set-definition-name -ps
+          type: string
+
+        - name: --policy-definition-name -pd


Short options can only be a single character.

tjprescott · 2018-05-17T21:53:02Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+    with self.argument_context('policy event') as c:
+        c.argument(
+            'management_group_name',
+            options_list=('--management-group-name', '-m'),


Please use list syntax (square brackets) and not tuple notation for options_list.

tjprescott · 2018-05-17T21:53:34Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+        c.argument(
+            'from_value',
+            options_list=('--from'),
+            help='Timestamp specifying the start time of the interval to query.')


What is the format of this? How are you (or are you?) validating this?

iso-8601. azure-mgmt-policyinsights API validates it to be iso-8601 and server validates it as well.

You should probably give the timestamp format as part of the parameter help

thanks. will do.

tjprescott · 2018-05-17T21:55:00Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+            'all_results',
+            options_list=('--all'),
+            arg_type=get_three_state_flag(),
+            help='Within the specified time interval, get all policy states instead of the latest only.')


Is this a flag on the command behavior or a Boolean property? three_state_flag is intended for Boolean properties only. https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#registering-flags

It is a Boolean parameter.

Since it isn't saved (it's not a Boolean property on a resource) this should probably not use three_state_flag but instead use just "action=store_true"

Thanks for the clarification. Will change this.

tjprescott · 2018-05-17T21:57:39Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+            help='Resource group name.')
+        c.argument(
+            'resource_id',
+            options_list=('--resource-id', '-r'),


If this can be an arbitrary resource, it should expose the full range of resource ID or named components. See:
https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#supporting-name-or-id-parameters

This makes the CLI inconsistent with the PS API, and ambiguous in itself. Our RP is an extension RP, and can be called with any resource as a scope. In the swagger API we specifically refrained from making parts of ARM resource ID separate parameters because that would make it impossible to support multiple nested level resources. API board review agreed on that. If we change this now in the CLI contract (so that we can support resource names), we will need to add multiple levels of namespace and resource type arguments, which makes the API unusable (and inconsistent with the PS API. Feel free to reach out to me to go over the swagger specs, PS API and any other details.

Consistency with Powershell is not important in the CLI, but consistency with other modules in the CLI is. This requires no change to swagger or the API.

As discussed offline, I'll implement additional parameters to make resourceid a resource that can take either an ID or a name.

tjprescott · 2018-05-17T21:59:04Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+        c.argument(
+            'to_value',
+            options_list=('--to'),
+            help='Timestamp specifying the end time of the interval to query.')


There seems to be a lot of duplication of arguments. Please review: https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#customize-arguments

Please register your arguments at the appropriate scopes to avoid copy/paste duplicate code.

This is unavoidable. "policy event" and "policy state" share those parameters, but those do not exist in "policy".

If there is a way to define them once and use in different 'with' statements, please let me know - this is the first time I'm touching Phthon code.

You can use a for loop:

for scope in ['state', 'event']: with self.argument_context('policy {}'.format(scope)) as c:

state has one additional parameter. can i add it to the context after this?

Looks like I can. thanks!

If a parameter doesn't exist in a scope, it is ignored, so you don't need to worry too much about extras.

Cool. Just to make this explicit, I did common ones in a for loop and then specifically added the extra one (--all) for state into the context. It worked.

tjprescott · 2018-05-17T22:00:12Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+        c.ignore('all_results')
+        c.ignore('order_by_clause')
+        c.ignore('select_clause')
+        c.ignore('apply_clause')


https://github.com/Azure/azure-cli/blob/dev/doc/authoring_command_modules/authoring_commands.md#customize-arguments

You can put all of these parameters in a single ignore entry.

tjprescott · 2018-05-17T22:03:08Z

...ommand_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/commands.py

+    )
+
+    with self.command_group('policy event', policy_events_sdk, client_factory=policy_events_operations) as g:
+        g.custom_command('list', 'list_policy_events')


These commands are going to live side-by-side with the policy commands defined in azure-cli-resource. Is there a reason you don't just add your commands to that module?

Yes, my intention is to maintain this package separately yet be part of the same command set. If the latter had not worked, I was planning to make it a separate command set.

Where do/will the powershell commands reside? If they are separate from the rest of the policy commands then this is also fine for us @twitchax

In PS, there is no command set concept. You can name your cmdlets the way you want following some naming conventions. We have 3 cmdlets in PS: Get-AzureRmPolicyEvent, Get-AzureRmPolicyState and Get-AzureRmPolicyStateSummary.

I want to maintain the package separately for our RP. Not having to name the command set policyinsights makes it consistent with PS as well. This is probably not important for CLI, but I think it makes it much easier for customers to remember.

We're fine with the new module in the CLI as in powershell, the policyinsights folder is also separate from that of resources

tjprescott · 2018-05-17T22:06:20Z

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

+            default_subscription_id,
+            query_options)
+
+    return summary.value[0]


What is the difference between summary and list?

List returns a list of policy states. Summarize returns pre-cooked aggregations as a summary object. Feel free to reach out to me if you need more info on the RP. I explained the RP to @williexu briefly.

bulentelmaci · 2018-05-18T00:04:40Z

@tjprescott @williexu Merged some of the requested changes. For the others and questions please see my comments above.

* Remove --subscription_id parameter * Change --all to be store_true * Change --resource_id to be --resource which can take resource ID or name * Add --resource-provider-namespace and --resource-type parameters that are required if --resource is not an ID

bulentelmaci · 2018-05-18T23:59:38Z

@tjprescott @williexu Updated the PR with the rest of the requested changes.

williexu · 2018-05-21T17:33:33Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+            c.argument(
+                'apply_clause',
+                options_list=['--apply'],
+                help='Apply expression for aggregations using OData notation.')


Use the arg_group kwarg to group similar arguments. Ex. "Resource Id", "Query", "Policy", etc.

williexu · 2018-05-21T17:33:36Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+                'resource_type',
+                options_list=['--resource-type', '-t'],
+                completer=get_resource_types_completion_list,
+                help='Resource type.')


Can you format the resource args to align more closely with that of az resource?
Help for resource show shown below:

(env36) C:\Users\wilx\azure-cli>az resource show -h Command az resource show: Get the details of a resource. Arguments --include-response-body: Use if the default command output doesn't capture all of the property data. Allowed values: false, true. Resource Id Arguments --api-version : The api version of the resource (omit for latest). --ids : One or more resource IDs (space-delimited). If provided, no other "Resource Id" arguments should be specified. --name -n : The resource name. (Ex: myC). --namespace : Provider namespace (Ex: 'Microsoft.Provider'). --parent : The parent path (Ex: 'resA/myA/resB/myB'). --resource-group -g : Name of resource group. You can configure the default group using `az configure --defaults group=<name>`. --resource-type : The resource type (Ex: 'resC'). Can also accept namespace/type format (Ex: 'Microsoft.Provider/resC').

As discussed, this does not apply to us.

I will make the changes to align other resource parameters with the resources command set (will change names and add a parent parameter)

Yep, ignore the --ids parameter and allow --resource to accept both an id and name.

Ok, thanks!

williexu · 2018-05-21T17:34:40Z

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

+            subscription_id,
+            query_options)
+
+    return summary.value[0]


Why do we only return the first value in the list?

The REST API path returns a list but the list always contains a single object (summary object).

williexu · 2018-05-21T17:34:52Z

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

+
+    subscription_id = get_subscription_id(cmd.cli_ctx)
+
+    if policy_assignment_name is not None:


Can the names be an empty string? If not, change this to if policy_assignment_name:.
Same for others below.

williexu · 2018-05-21T17:56:14Z

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

+                name=resource)
+        events = client.list_query_results_for_resource(
+            resource,
+            query_options)


Please provide a test for resource id scenario.
Also, what happens with more complex resource-ids like subnets? Here's an example id:
/subscriptions/<sub id>/resourceGroups/clitestlab/providers/Microsoft.Network/virtualNetworks/Dtlcliautomationlab/subnets/DtlcliautomationlabSubnet

As discussed, I will add a test for that with a hard coded resource id which will not be enabled for live execution.

* Add arg_group to all parameters * Don't override existing global resource_group_name parameter attributes other than arg_group; this way argument name and help stays standard with other commands. * Change the name of the resource provider namespace argument to make it consistent with resource package * Add parent argument to be used for resource name case to help specify nested resources * Handle empty string in arguments * Extend the scenario test to test all scopes and parameters; make it record only

williexu · 2018-05-22T20:48:36Z

rebase to get the updates from #6407 please

bulentelmaci · 2018-05-22T22:54:36Z

@tjprescott @williexu Updated the PR with the rest of the requested changes.

williexu

Looks generally good, small comments

williexu · 2018-05-22T23:59:48Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+                arg_group='Scope')
+            c.argument(
+                'resource',
+                options_list=['--resource', '-r'],


the -r option should not be given as it is not available for other instances of --resource in the CLI and the default option is relatively short

Forgot to remove. Will do.

williexu · 2018-05-22T23:59:50Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+                help='Resource type (Ex: ''resC'').')
+            c.argument(
+                'policy_set_definition_name',
+                options_list=['--policy-set-definition-name', '-p'],


-s for set would be a much better choice

Agreed. Will change it.

bulentelmaci · 2018-05-23T22:43:40Z

@tjprescott @williexu Made the small changes as well. Is this good to go?

bulentelmaci · 2018-05-23T22:45:05Z

@tjprescott @williexu When merged, do I need to do anything for publishing this to PyPl? If not, how long after the merge should I expect it to be available there for PIP install?

williexu · 2018-05-23T22:57:40Z

@bulentelmaci https://github.com/Azure/azure-cli/milestones has the dates of our release
Your command module will be released with the rest of the CLI on our release day

tjprescott

There's a major inconsistency between the "Resource ID" arguments and the "Scope" arguments. We should review options offline.

bulentelmaci · 2018-05-24T16:23:08Z

@tjprescott “Resource ID” which is one of the query scopes (like MG, subscription, RG, policy set/definition/assignment) for policy events and states has no direct association to the scope for a policy assignment. I’ll ping you offline to explain and discuss.

williexu

More small fixes that came to mind.
The options_lists for --resource and --resource-type don't need to be given. By default, we will convert underscore-separated arguments to the corresponding arguments that users see, i.e. resource_type to --resource-type

williexu · 2018-05-24T20:45:47Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+                options_list=['--resource'],
+                validator=validate_resource,
+                arg_group='Resource ID',
+                help='Resource ID or resource name.')


I'd recommend adding an extra line to the help along the lines of,
If an name is given, please provide the resource_group and other relevant resource id arguments.

This is because --resource-group will be under a different argument group.

williexu · 2018-05-24T20:49:32Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+                arg_group='Scope')
+            c.argument(
+                'resource',
+                options_list=['--resource'],


You can remove this options_list

williexu · 2018-05-24T20:49:41Z

...command_modules/azure-cli-policyinsights/azure/cli/command_modules/policyinsights/_params.py

+                help='The parent path (Ex: ''resA/myA/resB/myB'').')
+            c.argument(
+                'resource_type',
+                options_list=['--resource-type'],


…argument

williexu · 2018-05-24T20:59:41Z

@tjprescott for a last look

bulentelmaci added 11 commits May 10, 2018 17:33

Draft implementation of policy insights command module (no tests, par…

214f1ba

…ams, help, completers, etc.)

Various fixes (all commands and exception works now)

7d4e574

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

93aa726

Help, params and completers

34b1d7a

Integrate into existing "policy" command group (instead of a new "pol…

af11e06

…icyinsights" command group)

Add scenario test

f823652

Add __init__ files as well

b09ac30

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

9ab08f9

Add doc mapping

1c1a02c

Fix params and test

e6adc3e

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

12bc0c9

bulentelmaci added 3 commits May 17, 2018 12:24

Fix for CI errors

81bca6f

History.rst fix for CI failure

2d0abe9

More params and help fixes for CI failures

0a21e54

tjprescott suggested changes May 17, 2018

View reviewed changes

tjprescott added the New Service label May 17, 2018

Requested changes

caf23af

bulentelmaci added 2 commits May 18, 2018 16:57

PR feedback fixes

504ede2

* Remove --subscription_id parameter * Change --all to be store_true * Change --resource_id to be --resource which can take resource ID or name * Add --resource-provider-namespace and --resource-type parameters that are required if --resource is not an ID

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

ad373c9

precheck_pep8.sh fixes

eab48a8

williexu suggested changes May 21, 2018

View reviewed changes

bulentelmaci added 2 commits May 22, 2018 12:20

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

d71701f

bulentelmaci added 2 commits May 22, 2018 14:16

Merge branch 'dev' of https://github.com/Azure/azure-cli into dev

fdef1f7

Fix automation error caused by hardcoded subscription id in resourceid

3248fc0

williexu suggested changes May 23, 2018

View reviewed changes

Remove shortcut for --resource; change -p to -s

f44b7d3

williexu approved these changes May 23, 2018

View reviewed changes

Add to code owners

89a62f8

tjprescott suggested changes May 24, 2018

View reviewed changes

Remove -name from mg, and policy scope parameters

eae8d09

williexu reviewed May 24, 2018

View reviewed changes

Remove reduntant options_list instances; add more help to --resource …

dd1fa77

…argument

williexu approved these changes May 24, 2018

View reviewed changes

tjprescott approved these changes May 25, 2018

View reviewed changes

troydai merged commit f4ba5b0 into Azure:dev May 25, 2018

haroldrandom added the New Service label Oct 25, 2019


		subscription_id = get_subscription_id(cmd.cli_ctx)

		if policy_assignment_name is not None:

Command module for Microsoft.PolicyInsights #6378

Command module for Microsoft.PolicyInsights #6378

Uh oh!

Conversation

bulentelmaci commented May 17, 2018

Command module for the new Microsoft.PolicyInsights RP.

Uh oh!

promptws commented May 17, 2018

Uh oh!

codecov-io commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williexu May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

codecov-io commented May 17, 2018 •

edited

Loading

williexu May 21, 2018 •

edited

Loading