-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Specs for Microsoft.PolicyInsights GA version (2018-04-04) #2929
Conversation
…approval request pending) suppressions; add new suppression for equivalent paths error (approval request pending)
…body to a single string
…onalProperties; add @odata.count to operations; remove "type" from "rows" in first 2017-08-09-preview and add descriptions
…files for each version (one per namespace is sufficient as long as both files are fed into autorest as a bundle)
…ition (old API versions)
Reorganize files (remove resource type based subfolders; rename conflicting example files to have resource type name; change swagger spec files and MD file based on new file locations and names); change modelAsString for x-ms-enum to true based on API review board recommendation
Automation for azure-sdk-for-nodeNothing to generate for azure-sdk-for-node |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-libraries-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
AutoRest linter results for ARM Related Validation Errors/WarningsThese errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns. File: specification/policyinsights/resource-manager/readme.md
|
AutoRest linter results for SDK Related Validation Errors/WarningsThese errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns. File: specification/policyinsights/resource-manager/readme.md
|
@@ -30,7 +30,7 @@ | |||
} | |||
}, | |||
"paths": { | |||
"/{managementGroupId}/providers/Microsoft.PolicyInsights/policyEvents/{policyEventsResource}/queryResults": { | |||
"/providers/{managementGroupsNamespace}/managementGroups/{managementGroupName}/providers/Microsoft.PolicyInsights/policyEvents/{policyEventsResource}/queryResults": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this modeled as an enum instead of putting the value of the namespace in the URL itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autorest/oav validation does not like it. Errors out saying that a spec can only be defined for one namespace. This is the workaround I had found in our previous release. It results in identical API; since it is a single valued enum, it turns into a const string part of path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a note, there is no change here from previous version. The change you pointed out the comment on was done on the previous version to fix a different validation issue where multiple paths with matching template but different path parameter names (e.g. managementGroupId, vs. policyDefinitionId, etc.) was not acceptable.
} | ||
} | ||
}, | ||
"/{policySetDefinitionId}/providers/Microsoft.PolicyInsights/policyEvents/{policyEventsResource}/queryResults": { | ||
"/subscriptions/{subscriptionId}/providers/{authorizationNamespace}/policySetDefinitions/{policySetDefinitionName}/providers/Microsoft.PolicyInsights/policyEvents/{policyEventsResource}/queryResults": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authorizationnamespace - why an enum? instead of putting Microsoft.Authorization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see above.
@@ -1079,9 +1247,15 @@ | |||
} | |||
} | |||
}, | |||
"Operations": { | |||
"OperationsListResults": { | |||
"description": "List of available operations.", | |||
"properties": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema of this should comply with RPC - https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/proxy-api-reference.md#exposing-available-operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no change to this from the previous version. I am not able to see what is incompliant. Could you please point out specific parts?
Looks good. Signing off! |
"paths": { | ||
"/providers/{managementGroupsNamespace}/managementGroups/{managementGroupName}/providers/Microsoft.PolicyInsights/policyEvents/{policyEventsResource}/queryResults": { | ||
"post": { | ||
"operationId": "PolicyEvents_ListQueryResultsForManagementGroup", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PolicyEvents_ListQueryResultsForManagementGroup [](start = 24, length = 47)
Could this be a pageable operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support paging right now. There is no change in this version from the previous one with respect to that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the returing schema does not need to have a next link. In the extension, the next link can be set to null, but still generate cool code to iterate on the array.
WDYT?
See example 3
https://github.com/Azure/autorest/tree/master/docs/extensions#x-ms-pageable
In reply to: 184759379 [](ancestors = 184759379)
}, | ||
{ | ||
"$ref": "#/parameters/applyParameter" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about grouping parameters? https://github.com/Azure/autorest/tree/master/docs/extensions#x-ms-parameter-grouping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used it. See QueryOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"QueryFailure": { | ||
"description": "Error response.", | ||
"properties": { | ||
"error": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error [](start = 9, length = 5)
I am not a big fan of defining stuff inline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found this easier to read since there are just two parameters and is extremely unlikely to change in the future (all of ARM is using the same). If you are feeling strong about it I can change; I just want to avoid changes from the previous specs if I can to minimize risk here.
}, | ||
"/providers/Microsoft.PolicyInsights/operations": { | ||
"get": { | ||
"operationId": "Operations_List", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operations_List [](start = 24, length = 15)
Pageable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pageability is planned for future (we have backend issues to be resolved for this for us to expose it in the API)
Adding swagger specs for Microsoft.PolicyInsights GA version "2018-04-04". The specs are identical to the latest preview version "2017-12-12-preview", except addition of a property to response schema of */summarize paths (PolicyDefinitionSummary schema has the new property "policyDefinitionReferenceId").
Also added to readme.md and made it the default version.
Please diff 2017-12-12-preview specs with 2018-04-04 specs to see the limited set of changes.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger