Add built in policy definition/set definition paths #1788

vivsriaus · 2017-10-03T21:17:50Z

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.

pilor · 2017-10-03T21:48:50Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

+          }
+        ],
+        "responses": {
+          "200": {


404 should be valid response correct?

pilor · 2017-10-03T21:49:27Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

+        "description": "Gets all the built in policy definitions.",
+        "parameters": [
+          {
+            "name": "$filter",


policyDefinition collection requests don't support odata fliters. Only policyAssignments have filters

pilor · 2017-10-03T21:49:47Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

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


"Returns an"

lmazuel · 2017-10-03T22:37:11Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

@@ -256,13 +279,6 @@
        "description": "Gets all the policy definitions for a subscription.",
        "parameters": [
          {
-            "name": "$filter",


So this was not supported and here by mistake?

Yes, apparently, only policy assignment list supports filter. Not policy definitions and set definitions

ok, this was exposed in the CLI, I create an issue for that

Azure/azure-cli#4603

lmazuel · 2017-10-03T22:38:49Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

@@ -292,19 +332,9 @@
        "description": "Gets all the policy definitions for a subscription at management group level.",
        "parameters": [
          {
-            "name": "$filter",


Same question, not supported?

yes, not supported

veronicagg · 2017-10-03T23:13:07Z

@ravbhatnagar few apis added to existing api versions, some properties removed. Can you please review from ARM side? thanks!

vivsriaus · 2017-10-03T23:16:26Z

@veronicagg Adding @cyl3392207, the PM for policy related ARM apis

veronicagg

Are the added apis new to the existing api versions or were there missing in the specs?
Please take a look at model validation results at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957464#L577
Have you taken a look at linter results https://travis-ci.org/Azure/azure-rest-api-specs/jobs/282957459#L579 ?

veronicagg · 2017-10-03T23:07:02Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

@@ -137,6 +137,38 @@
        }
      }
    },
+    "/providers/Microsoft.Authorization/policydefinitions/{policyDefinitionName}": {


was this api missing from the spec? or is it newly added to the existing api version?

this was missing from the spec

veronicagg · 2017-10-03T23:08:35Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

+        }
+      }
+    },
+    "/providers/Microsoft.Authorization/policydefinitions": {


was this api missing from the spec before but existed in the service?

yes, we never added this to spec

perseusCode

vivsriaus · 2017-10-04T18:35:39Z

@veronicagg The model validation results are because of missing example for 404 response, which I think is redundant here, and doesn't add any value. The linter result errors are also because of missing examples.

azuresdkciprbot · 2017-10-04T21:16:59Z

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:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

cyl33922072 · 2017-10-04T22:01:19Z

Approve from my side.

ravbhatnagar

@vivsriaus @veronicagg looks good. one minor thing.

ravbhatnagar · 2017-10-04T22:49:21Z

...ication/resources/resource-manager/Microsoft.Authorization/2016-12-01/policyDefinitions.json

@@ -137,6 +137,38 @@
        }
      }
    },
+    "/providers/Microsoft.Authorization/policydefinitions/{policyDefinitionName}": {


camel case ->policyDefinitions

azuresdkciprbot · 2017-10-04T22:56:49Z

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:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

veronicagg

Looks good, some missing examples that based on offline conversation will be added in a later PR.

AutorestCI · 2017-10-04T23:03:50Z

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

AutorestCI · 2017-10-04T23:05:14Z

Azure/azure-sdk-for-ruby#1019

AutorestCI · 2017-10-04T23:05:39Z

Azure/azure-sdk-for-python#1507

cyl33922072 · 2017-10-06T22:37:41Z

Approve

vivsriaus added 3 commits October 3, 2017 14:11

Add built in policy definitions paths

503e9fe

Merge

3abdae4

Add built in policy definitions paths

4288c9c

azuresdkci assigned veronicagg Oct 3, 2017

pilor suggested changes Oct 3, 2017

View reviewed changes

vivsriaus added 4 commits October 3, 2017 14:51

Remove unnecessary subId parameter from MG paths

a0a081a

Remove unnecessary subId parameter from MG paths

5da03ba

404 is valid response, remove filter parameter from list operations

91c7775

Add support for getting/listing buil in policy set definitions

26ebb96

pilor approved these changes Oct 3, 2017

View reviewed changes

Remove x-ms-odata for list policy definitions and set definitions

75b1c0c

vivsriaus changed the title ~~Add built in policy definition paths~~ Add built in policy definition/set definition paths Oct 3, 2017

vivsriaus mentioned this pull request Oct 3, 2017

[Resource/Policy] Add support for policy at ManagementGroup level Azure/azure-sdk-for-python#1499

Closed

lmazuel reviewed Oct 3, 2017

View reviewed changes

lmazuel mentioned this pull request Oct 3, 2017

"az policy definition list" argument "--filter" is not supported by Azure Azure/azure-cli#4603

Closed

veronicagg added potential-sdk-breaking-change WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Oct 3, 2017

veronicagg suggested changes Oct 3, 2017

View reviewed changes

vishrutshah mentioned this pull request Oct 3, 2017

[Travis CI] Azure PR Bot CI job must fail on the JSON.parse error from linter output #1792

Closed

perseusCode approved these changes Oct 4, 2017

View reviewed changes

Add 404 response to example to get the CI model validation job to pass

cf054dd

ravbhatnagar approved these changes Oct 4, 2017

View reviewed changes

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Oct 4, 2017

Camel case policyDefinitions and policySetDefinitions in paths

730e2ee

veronicagg approved these changes Oct 4, 2017

View reviewed changes

veronicagg merged commit 9f0f02a into Azure:current Oct 4, 2017

AutorestCI mentioned this pull request Oct 4, 2017

Automatic PR from RestAPI-PR1788 Azure/azure-sdk-for-ruby#1019

Closed

AutorestCI mentioned this pull request Oct 4, 2017

[Res/Policy] Add built in policy definition/set definition paths Azure/azure-sdk-for-python#1507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add built in policy definition/set definition paths #1788

Add built in policy definition/set definition paths #1788

vivsriaus commented Oct 3, 2017 •

edited by azuresdkci

Loading

pilor Oct 3, 2017

vivsriaus Oct 3, 2017

pilor Oct 3, 2017

vivsriaus Oct 3, 2017

pilor Oct 3, 2017

vivsriaus Oct 3, 2017

lmazuel Oct 3, 2017

vivsriaus Oct 3, 2017

lmazuel Oct 3, 2017

lmazuel Oct 3, 2017

lmazuel Oct 3, 2017

vivsriaus Oct 3, 2017

veronicagg commented Oct 3, 2017

vivsriaus commented Oct 3, 2017

veronicagg left a comment

veronicagg Oct 3, 2017

vivsriaus Oct 3, 2017

veronicagg Oct 3, 2017

vivsriaus Oct 3, 2017

perseusCode left a comment

vivsriaus commented Oct 4, 2017

azuresdkciprbot commented Oct 4, 2017

cyl33922072 commented Oct 4, 2017

ravbhatnagar left a comment

ravbhatnagar Oct 4, 2017

azuresdkciprbot commented Oct 4, 2017

veronicagg left a comment

AutorestCI commented Oct 4, 2017

AutorestCI commented Oct 4, 2017

AutorestCI commented Oct 4, 2017

cyl33922072 commented Oct 6, 2017

Add built in policy definition/set definition paths #1788

Add built in policy definition/set definition paths #1788

Conversation

vivsriaus commented Oct 3, 2017 • edited by azuresdkci Loading

PR information

Quality of Swagger

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

veronicagg commented Oct 3, 2017

vivsriaus commented Oct 3, 2017

veronicagg left a 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

perseusCode left a comment

Choose a reason for hiding this comment

vivsriaus commented Oct 4, 2017

azuresdkciprbot commented Oct 4, 2017

cyl33922072 commented Oct 4, 2017

ravbhatnagar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azuresdkciprbot commented Oct 4, 2017

veronicagg left a comment

Choose a reason for hiding this comment

AutorestCI commented Oct 4, 2017

AutorestCI commented Oct 4, 2017

AutorestCI commented Oct 4, 2017

cyl33922072 commented Oct 6, 2017

vivsriaus commented Oct 3, 2017 •

edited by azuresdkci

Loading