Skip to content

Update Azure Governance Policy api-version to 2018-03-01#3032

Merged
marstr merged 9 commits intoAzure:masterfrom
mentat9:master
May 16, 2018
Merged

Update Azure Governance Policy api-version to 2018-03-01#3032
marstr merged 9 commits intoAzure:masterfrom
mentat9:master

Conversation

@mentat9
Copy link
Member

@mentat9 mentat9 commented May 7, 2018

This api spec combines api-version 2017-06-01-preview and 2016-12-01 into one, cleans up all validation issues, adds full examples, and clarifies/improves all descriptions. It should be fully back-compatible.

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

mentat9 added 2 commits May 4, 2018 15:41
 - add many missing examples
 - improve/complete descriptions and summaries
Add/correct some response codes
@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@AutorestCI
Copy link

AutorestCI commented May 7, 2018

Automation for azure-sdk-for-node

Encountered a Subprocess error: (azure-sdk-for-node)

Command: ['/usr/local/bin/autorest', '/tmp/tmp6rn5la_d/rest/specification/resources/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmp6rn5la_d/sdk', '--nodejs', '--use=@microsoft.azure/autorest.nodejs@^2.1.43']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4275/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4275)
   Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (^2.1.43->2.1.59)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
[Exception] No input files provided.

Use --help to get help information.

Copy link
Member

@marstr marstr left a comment

Choose a reason for hiding this comment

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

Our CI system is pretty unhappy with this PR. Before I review this in earnest, could you fix the invalid JSON that is likely messing with our tooling?
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/376107691#L699

@AutorestCI
Copy link

AutorestCI commented May 8, 2018

Automation for azure-libraries-for-java

The initial PR has been merged into your service PR:
AutorestCI/azure-libraries-for-java#154

@AutorestCI
Copy link

AutorestCI commented May 8, 2018

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#1839

@AutorestCI
Copy link

AutorestCI commented May 8, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2570

@mentat9
Copy link
Member Author

mentat9 commented May 8, 2018

I'd love to. Thing is, that file exists and is completely valid JSON: https://github.com/mentat9/azure-rest-api-specs/blob/master/specification/resources/resource-manager/Microsoft.Authorization/stable/2018-03-01/examples/listBuiltInPolicyDefinitions.json

I haven't learned all the quirks of Travis, but it looks like it's detecting something different than what the message says. I don't see anything wrong with that file, or anything wrong in the nearby JSON. All of the local validation tools find that file and validate it just fine. Have you come across this kind of error before?

@mentat9
Copy link
Member Author

mentat9 commented May 8, 2018

Adding @pilor

Change policyAssignment result for nonexistent delete by ID to match all other delete methods
Attempt to fix Travis FNF error
@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@mentat9
Copy link
Member Author

mentat9 commented May 8, 2018

OK, I've fixed the Travis issue. Turns out sometimes JSON references to external files interpret the file path as case sensitive. Autorest running against a local repo on my dev box resolves $ref as case INsensitive, but when it runs against a GitHub repo, it resolves that same $ref as case sensitive. This is a bad tool bug, IMO.

},
"policyDefinitions": [
{
"policyDefinitionId": "/subscriptions/ae640e6b-ba3e-4256-9d62-2993eecfa6f2/providers/Microsoft.Authorization/policyDefinitions/7433c107-6db4-4ad1-b57a-a76dce0154a1",
Copy link
Contributor

Choose a reason for hiding this comment

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

an MG policySet wouldn't be able to reference a policyDefinition in a subscription (can only reference at the same level or above). Should change this to an MG level reference for completeness

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, will do.

@@ -0,0 +1,51 @@
{
"parameters": {
"policyAssignmentId": "providers/Microsoft.Management/managementGroups/MyManagementGroup/providers/Microsoft.Authorization/policyAssignments/LowCostStorage",
Copy link
Contributor

Choose a reason for hiding this comment

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

leading slash?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, the slash is in the path template: /{policyAssignmentId}. Including it in the parameter would create a double slash. Similar thing happens in examples of the /{scope}/. . . path.

@@ -0,0 +1,36 @@
{
"parameters": {
"policyAssignmentId": "providers/Microsoft.Management/managementGroups/MyManagementGroup/providers/Microsoft.Authorization/policyAssignments/LowCostStorage",
Copy link
Contributor

Choose a reason for hiding this comment

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

leading slash?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same

"resourceName": "MyTestComputer.cloudapp.net",
"subscriptionId": "ae640e6b-ba3e-4256-9d62-2993eecfa6f2",
"api-version": "2018-03-01",
"$filter": "atScope()"
Copy link
Contributor

Choose a reason for hiding this comment

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

if atScope() was provided only policies that were assigned directly on the resource would be returned, the response below has subscription level resources

Copy link
Member Author

Choose a reason for hiding this comment

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

Quite right. I'll get it fixed up.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@marstr marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label May 9, 2018
@ravbhatnagar
Copy link
Contributor

@marstr @mentat9 - Its hard to review the changes in this api-version. The first commit didnt include the original version.

@ravbhatnagar
Copy link
Contributor

Ok, so I compared the policyAssignment.json, policyDefinition.json with the previos version using a diff tool. There were no major changes. I also took a look at policySetDefinition.json which is a new file. APIs for this are already live on service side. So signing off from ARM side.

@ravbhatnagar 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 May 15, 2018
@mentat9
Copy link
Member Author

mentat9 commented May 15, 2018

@marstr, all checks are passing: can you close your change request/comment?

@marstr
Copy link
Member

marstr commented May 15, 2018

@mentat9, are these changes deployed from your side, and ready for me to merge?

@mentat9
Copy link
Member Author

mentat9 commented May 16, 2018

Actually, I just discovered some wording changes I want to make in description fields. I'll ping back after those are done.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/resources/resource-manager/readme.md

⚠️0 new Warnings.(0 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@mentat9
Copy link
Member Author

mentat9 commented May 16, 2018

@marstr, We are good to go from a service perspective, and my description updates are in. Please merge at your earliest convenience after checks succeed.

@marstr marstr merged commit a2422bb into Azure:master May 16, 2018
@mentat9
Copy link
Member Author

mentat9 commented Jun 6, 2018

@marstr do you happen to know whom I should follow this failure up with? I need to generate the Node.js SDK, but the docs give an email alias that no longer exists for the Node.js SDK team.

Automation for azure-sdk-for-node

Encountered a Subprocess error: (azure-sdk-for-node)

Command: ['/usr/local/bin/autorest', '/tmp/tmp6rn5la_d/rest/specification/resources/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmp6rn5la_d/sdk', '--nodejs', '--use=@microsoft.azure/autorest.nodejs@^2.1.43']
Finished with return code 1
and output:
AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Loading AutoRest core '/root/.autorest/@microsoft.azure_autorest-core@2.0.4275/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4275)
Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (^2.1.43->2.1.59)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
[Exception] No input files provided.

Use --help to get help information.

@RikkiGibson
Copy link
Member

@AutorestCI rebuild azure-sdk-for-node

@AutorestCI
Copy link

Encountered a Subprocess error

Command: ['/usr/local/bin/autorest', '/tmp/tmpda054lkj/rest/specification/resources/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmpda054lkj/sdk', '--nodejs', '--use=@microsoft.azure/autorest.nodejs@^2.1.43']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4278/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4278)
   Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (^2.1.43->2.1.59)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
[Exception] No input files provided.

Use --help to get help information.

@RikkiGibson
Copy link
Member

@AutorestCI rebuild azure-sdk-for-node

@AutorestCI
Copy link

Encountered a Subprocess error

Command: ['/usr/local/bin/autorest', '/tmp/tmpnpmjnkn8/rest/specification/resources/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmpnpmjnkn8/sdk', '--nodejs', '--use=@microsoft.azure/autorest.nodejs@^2.1.43']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4280; node: v8.11.3]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4280/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
   Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (^2.1.43->2.1.59)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
[Exception] No input files provided.

Use --help to get help information.

@RikkiGibson
Copy link
Member

Really not sure what the problem is-- when I copy those args, modify to my local versions of the SDK and rest-api-specs and run locally, it's fine. I'm opening a PR for you @mentat9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants