Add policy set definition APIs #1587

vivsriaus · 2017-08-24T23:03:14Z

Add policy set definition APIs, add notScopes to policy assignment, add policyMode to policy definition, update the api-version

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

vivsriaus · 2017-08-24T23:05:58Z

@gandhiniraj @cyl3392207 Please review this one instead @vishrutshah Mike and Niraj are from the ARM team, who can review this. So is @brendandburns who signed off on the previous PR

vishrutshah · 2017-08-25T00:06:30Z

@veronicagg Taking it over to my plate as i made them close and re-open new one :)

@vivsriaus Thanks for adding two commits. We'll review it as soon as ARM team signs-off.

vishrutshah · 2017-08-25T00:10:01Z

@ravbhatnagar Let me know if this has been gone through ARM sign-off.

@vivsriaus Meanwhile, would you mind please providing us with the x-ms-examples for new operations to help us with model validation. Thanks!

… into AddPolicySetNew

vivsriaus · 2017-08-25T19:36:19Z

@vishrutshah Examples added. Thanks for catching that!

… into AddPolicySetNew

vivsriaus · 2017-08-28T18:40:59Z

@vishrutshah Let me know if this looks good to be merged. Thanks

… into AddPolicySetNew

vishrutshah · 2017-08-29T00:29:27Z

@ravbhatnagar Could you please review this from ARM perspective, please? Thanks!

ravbhatnagar · 2017-08-29T06:58:42Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

-    },  
-     "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{parentResourcePath}/{resourceType}/{resourceName}/providers/Microsoft.Authorization/policyassignments": {
+    },
+    "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{parentResourcePath}/{resourceType}/{resourceName}/providers/Microsoft.Authorization/policyassignments": {


camelcase policyAssignments

ravbhatnagar · 2017-08-29T06:58:56Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

        }
      }
    },
+    "/subscriptions/{subscriptionId}/providers/Microsoft.Authorization/policysetdefinitions/{policySetDefinitionName}": {


camelcasing

ravbhatnagar · 2017-08-29T07:01:21Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+          "$ref": "#/definitions/PolicySetDefinitionProperties",
+          "description": "The policy definition properties."
+        },
+        "id": {


ravbhatnagar · 2017-08-29T07:01:28Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+          "type": "string",
+          "description": "The ID of the policy set definition."
+        },
+        "name": {


name should be settable

name comes from the URL. It is not settable in the request body

you can pass name in the request body, and we throw an error when the passed in name doesn't match what is in the Url

ravbhatnagar · 2017-08-29T07:01:45Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+          "type": "string",
+          "description": "The name of the policy set definition. If you do not specify a value for name, the value is inferred from the name value in the request URI."
+        }
+      },


type property missing.

ravbhatnagar · 2017-08-29T07:06:08Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+        },
+        "x-ms-odata": "#/definitions/PolicySetDefinition"
+      }
+    },


Patch API missing?

I don't see a PATCH in our source code

ravbhatnagar · 2017-08-29T07:07:40Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+            "$ref": "#/parameters/SubscriptionIdParameter"
+          }
+        ],
+        "responses": {


Please add a default response to capture all the error responses. The shape of the error object complies with RPC. Example here - https://github.com/Azure/azure-rest-api-specs/blob/current/specification/batch/resource-manager/Microsoft.Batch/2017-05-01/BatchManagement.json#L101

ravbhatnagar · 2017-08-29T07:09:02Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+            "$ref": "#/parameters/SubscriptionIdParameter"
+          }
+        ],
+        "responses": {


The description of 204 can state that the policysetDefinition does not exist in the subscription

We usually use the http code description in our specs, so I'm keeping it consistent across the board. For instance, like this.

This is something that we can improve across the board then. Lets start with the new APIs we are adding. More/clear description is always better :)

ravbhatnagar · 2017-08-29T07:10:18Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+        "description": "Gets all the policy set definitions for a subscription.",
+        "parameters": [
+          {
+            "name": "$filter",


What is supported through $filter. It would be good if the common cases are called out in the description.

ravbhatnagar · 2017-08-29T07:11:16Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

      "description": "The policy definition properties."
    },
+    "PolicySetDefinitionProperties": {
+      "properties": {


do you need a provisioningState property?

I don't think we need it

If the creates are not long running than its fine.

… into AddPolicySetNew

vivsriaus · 2017-09-05T19:39:25Z

@vishrutshah The CI is failing because it says ErrorResponse is not allowed as part of responses? We had default error responses for our other spec file (here) and didn't receive any validation errors then. Can you please take a look?

pilor · 2017-09-06T18:27:34Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+        },
+        "parameters": {
+          "type": "object",
+          "description": "Required if a parameter is used in policy rule."


This description should be updated as it is not applicable to setDefinitions. Should be something along the lines of "The policy set definition parameters that can be used in policy definition references"

pilor · 2017-09-06T18:30:49Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+  "swagger": "2.0",
+  "info": {
+    "title": "PolicyClient",
+    "version": "2017-06-01-preview",


policyDefinitions don't have this api-version (though they probably should have with mode added). I think that means you need to split out the swagger specs from 2016-12-01 and combine policyDefinitions there with policyAssignments and policySetDefinitions here.

I don't understand. policyDefinitions APIs won't work with 2017-06-01-preview version?

Correct, api-versions are resourceType specific and policyDefinitions did not have a change that the people who made the changes thought warranted a new api-version. Calls to its APIs with 2017-06-01-preview will not work

Ah, I see. Before I go ahead and do the split, shouldn't we bump the api-version for policyDefinitions as well, now that we have mode?

Mode was already released to prod on all existing api-versions so it could be even more breaking to restrict it now (even though its probably not used by anyone yet). There is the option of introducing a new api-version on policyDefinitions just to make this work easier but I don't know if that's a great idea. It will be good to have them split in the future so they can be incremented individually when needed. If everything gets a new api-version every time anything changes you end up with a ton of api-versions that user has to wade through.

See Microsoft.SQL for an example of resource types that were split out into their own swagger specs

pilor · 2017-09-06T18:35:08Z

...r/Microsoft.Authorization/2017-06-01-preview/examples/createOrUpdatePolicySetDefinition.json

+        "description": "Policies required to minimize the risk of accidental cost overruns",
+        "metadata": {
+          "category": "Cost Management"
+        },


example should include parameters since they are typically the most complex part of these objects

pilor · 2017-09-06T18:40:11Z

...urce-manager/Microsoft.Authorization/2017-06-01-preview/examples/getPolicySetDefinition.json

+              "parameters": {
+                "listOfAllowedSKUs": {
+                  "value": [
+                    "“Standard_GRS”",


both of these values should just have 1 "

pilor · 2017-09-06T18:40:56Z

...ce-manager/Microsoft.Authorization/2017-06-01-preview/examples/listPolicySetDefinitions.json

+                  "parameters": {
+                    "listOfAllowedSKUs": {
+                      "value": [
+                        "“Standard_GRS”",


pilor · 2017-09-06T18:53:54Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+          "type": "string",
+          "description": "The display name of the policy definition."
+        },
+        "description": {


policyDefinition supports metadata now

pilor · 2017-09-06T18:54:12Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+          },
+          "description": "The policy's excluded scopes."
+        },
+        "parameters": {


policyAssignment supports metadata

pilor · 2017-09-06T18:55:13Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+        },
+        "policyDefinitionId": {
+          "type": "string",
+          "description": "The ID of the policy definition."


Can the description be "The ID of the policy definition or policy set definition"

pilor · 2017-09-06T19:15:23Z

...r/Microsoft.Authorization/2017-06-01-preview/examples/createOrUpdatePolicySetDefinition.json

@@ -0,0 +1,44 @@
+{
+  "parameters": {


these example files need to be referenced in the swagger spec for them to show up in documentation, right? See https://github.com/pilor/azure-rest-api-specs/blob/current/documentation/x-ms-examples.md

Of course they should! thanks for catching this

pilor · 2017-09-06T19:16:11Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

@@ -0,0 +1,1102 @@
+{


This new spec needs to be added to the root readme.md to have it included in autorest packages, correct?

good catch!

… into AddPolicySetNew

vishrutshah

Left some minor comments. I'll wait for the build to complete to check on validation results on semantic, model and linter results.

vishrutshah · 2017-09-06T20:11:44Z

specification/resources/resource-manager/readme.md

 - Microsoft.Authorization/2015-01-01/locks.json
 ```

+### Tag: package-policy-2017-06


Will you be having the api-version 2017-06-01 in future without -preview in api-version? In that case, I'd recommend tag to reflect the -preview.

vishrutshah · 2017-09-06T20:35:23Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+          ],
+          "responses": {
+            "204": {
+              "description": "No Content"


Please provide better descriptions as per recommendation from Gaurav that'd not only help SDK but also better API documentations as well

vishrutshah · 2017-09-06T20:40:26Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+        ],
+        "responses": {
+          "200": {
+            "description": "OK - Returns ana array of policy definitions.",


typo in ana

vishrutshah

Please review model validation errors https://travis-ci.org/Azure/azure-rest-api-specs/jobs/272643689#L670 Thanks!

vishrutshah · 2017-09-06T21:53:00Z

specification/resources/resource-manager/Microsoft.Authorization/2017-06-01-preview/policy.json

+      "description": "The policy definition.",
+      "x-ms-azure-resource": true
+    },
+    "PolicySetDefinition": {


Seems like this model is missing property named as type from the example.

https://travis-ci.org/Azure/azure-rest-api-specs/jobs/272643689#L691

… into AddPolicySetNew

…version

pilor · 2017-09-07T23:12:41Z

...ce-manager/Microsoft.Authorization/2017-06-01-preview/examples/listPolicySetDefinitions.json

+                  "policyDefinitionId": "/subscriptions/subid/providers/Microsoft.Authorization/policyDefinitions/storageSkus",
+                  "parameters": {
+                    "allowedLocations": {
+                      "value": [


To make this clear and valid can you change this to "parameters": { "validLocations": { "value": "[parameters('allowedLocations')]" } }?

pilor · 2017-09-07T23:14:19Z

specification/resources/resource-manager/readme.md

 - Microsoft.Authorization/2015-01-01/locks.json
 ```

+### Tag: package-policy-2017-06


line 40 should change to reference this new tag

pilor

Just added 2 more minor comments

azuresdkciprbot · 2017-09-07T23:43:56Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/resources/resource-manager/readme.md
Before the PR: Warning(s): 28 Error(s): 47
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

vishrutshah · 2017-09-08T18:12:38Z

Please review the model validation error here https://travis-ci.org/Azure/azure-rest-api-specs/jobs/273107502#L715

vivsriaus · 2017-09-08T18:17:57Z

@vishrutshah the error says valid example for response 200 is missing for createOrUpdatePolicySetDefinition. The response would be the same, so I don't think it warrants a separate example. Agree?

vivsriaus · 2017-09-11T17:31:36Z

@vishrutshah can you please look into this?

vishrutshah · 2017-09-11T19:46:06Z

Hi @vivsriaus, regarding

the error says valid example for response 200 is missing for createOrUpdatePolicySetDefinition. The response would be the same, so I don't think it warrants a separate example. Agree?

We'd like to make the examples complete. That menas that you'd define the sample responses for all the response code defined in the swagger. Otherwise tool cannot validate whether provided example is complete or not just based on the model are same. It's be great if you can provide sample response in example for code 200 as well as that is one of the positive response code defined in swagger and model validator would like to validate, please. Thanks! Other changes LGTM!

… into AddPolicySetNew

azuresdkciprbot · 2017-09-11T20:54:05Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/resources/resource-manager/readme.md
Before the PR: Warning(s): 28 Error(s): 47
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

AutorestCI · 2017-09-11T23:33:24Z

Azure/azure-sdk-for-python#1434

AutorestCI · 2017-09-11T23:33:29Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-09-11T23:34:49Z

Azure/azure-sdk-for-ruby#970

vivsriaus added 2 commits August 24, 2017 16:00

Add existing policy json to make review easier

7c4eedd

Add policy set definition APIs

cdf4468

msftclas added the cla-not-required label Aug 24, 2017

vivsriaus mentioned this pull request Aug 24, 2017

Add policy set definition APIs #1585

Closed

10 tasks

azuresdkci assigned veronicagg Aug 24, 2017

vishrutshah assigned vishrutshah and unassigned veronicagg Aug 25, 2017

vishrutshah added ARM WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 25, 2017

vivsriaus added 2 commits August 25, 2017 11:20

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

e9d9720

… into AddPolicySetNew

Add examples for policySetDefinition APIs

59a4fcc

vivsriaus added 2 commits August 25, 2017 12:52

Add policySku to policy assignment

ddd97a0

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

286e4a1

… into AddPolicySetNew

cyl33922072 approved these changes Aug 25, 2017

View reviewed changes

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

6b13491

… into AddPolicySetNew

ravbhatnagar suggested changes Aug 29, 2017

View reviewed changes

ravbhatnagar added ReadyForSDKReview and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 29, 2017

vivsriaus added 3 commits August 30, 2017 11:31

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

ff2f6c8

… into AddPolicySetNew

Address feedback

139ee8d

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

28b7136

… into AddPolicySetNew

pilor suggested changes Sep 6, 2017

View reviewed changes

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

5721e81

… into AddPolicySetNew

vivsriaus added 2 commits September 6, 2017 12:57

Add parameter example

b37a167

Fix incorrect example in listPSDefinitions

290fd9b

vishrutshah suggested changes Sep 6, 2017

View reviewed changes

vivsriaus added 4 commits September 7, 2017 15:00

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

ddd44e0

… into AddPolicySetNew

Remove policy definitions from latest api version

0ceabfc

Update policy definition and policy assignment for old api-version

df1297f

Update readme.md to include multiple policy files for new policy api …

ea7c1ed

…version

pilor reviewed Sep 7, 2017

View reviewed changes

pilor approved these changes Sep 7, 2017

View reviewed changes

Address more feedback

780cc26

vivsriaus added 2 commits September 11, 2017 13:42

Merge branch 'current' of https://github.com/Azure/azure-rest-api-specs…

5bd98a3

… into AddPolicySetNew

Add example for 200

f7776f4

vishrutshah approved these changes Sep 11, 2017

View reviewed changes

ravbhatnagar approved these changes Sep 11, 2017

View reviewed changes

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Sep 11, 2017

vishrutshah merged commit 40e8139 into Azure:current Sep 11, 2017

AutorestCI mentioned this pull request Sep 11, 2017

[Resource/Policy] Add policy set definition APIs Azure/azure-sdk-for-python#1434

Closed

AutorestCI mentioned this pull request Sep 11, 2017

Automatic PR from RestAPI-PR1587 Azure/azure-sdk-for-ruby#970

Merged

Add policy set definition APIs #1587

Add policy set definition APIs #1587

Uh oh!

Conversation

vivsriaus commented Aug 24, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

vivsriaus commented Aug 24, 2017

Uh oh!

vishrutshah commented Aug 25, 2017

Uh oh!

vishrutshah commented Aug 25, 2017

Uh oh!

vivsriaus commented Aug 25, 2017

Uh oh!

vivsriaus commented Aug 28, 2017

Uh oh!

vishrutshah commented Aug 29, 2017

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!

vivsriaus commented Sep 5, 2017

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!

pilor Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

vivsriaus commented Aug 24, 2017 •

edited by azuresdkci

Loading

pilor Sep 6, 2017 •

edited

Loading