-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Azure Purview Policy Store Self-service Policies Spec #21750
Conversation
Hi, @saranya-azuredev Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vscswagger@microsoft.com |
Swagger Validation Report
|
compared tags (via openapi-validator v2.0.0) | new version | base version |
---|---|---|
package-2022-12-01-preview | package-2022-12-01-preview(ca559df) | default(main) |
[must fix]The following errors/warnings are introduced by current PR:
Rule | Message | Related RPC [For API reviewers] |
---|---|---|
Path parameter should specify a maximum length (maxLength) and characters allowed (pattern). Location: Azure.Analytics.Purview.SelfServicePolicies/preview/2022-12-01-preview/purviewSelfServicePolicy.json#L65 |
||
Path parameter should specify characters allowed (pattern). Location: Azure.Analytics.Purview.SelfServicePolicies/preview/2022-12-01-preview/purviewSelfServicePolicy.json#L109 |
️️✔️
Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️
ApiReadinessCheck succeeded [Detail] [Expand]
️⚠️
~[Staging] ServiceAPIReadinessTest: 0 Warnings warning [Detail]
API Test is not triggered due to precheck failure. Check pipeline log for details.
️️✔️
~[Staging] SwaggerAPIView succeeded [Detail] [Expand]
️️✔️
~[Staging] CadlAPIView succeeded [Detail] [Expand]
️️✔️
ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️
SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️
PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️
PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️
SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️
Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️
CadlValidation succeeded [Detail] [Expand]
Validation passes for CadlValidation.
️️✔️
PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
Swagger Generation Artifacts
|
Generated ApiView
|
Hi, @saranya-azuredev, For review efficiency consideration, when creating a new api version, it is required to place API specs of the base version in the first commit, and push new version updates into successive commits. You can use OpenAPIHub to initialize the PR for adding a new version. For more details refer to the wiki. Or you could onboard API spec pipeline |
Hi @saranya-azuredev, Your PR has some issues. Please fix the CI sequentially by following the order of
|
@mikekistler kindly review the PR |
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.
Style-wise this looks very nice! I noted a few minor points that should be easy to fix.
], | ||
"type": "object", | ||
"properties": { | ||
"workflowRunId": { |
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.
You should not call this property workflowRunId
or describe it as the id of a workflow. This is the id of the selfServicePolicy so it should be called something like policyId
. Your service might derive it from the workflowRunId but this is an internal detail that should not leak out into the API contract.
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.
Have modified the field to policyId instead of workflowRunId.
"type": "string", | ||
"description": "The requestor of the self-service policy" | ||
}, | ||
"expiryDate": { |
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.
It is good practice to have a naming convention for date-time properties (we are working an update to the Azure REST API guidelines for this). This file has 3 date-time properties and the other two end with "At", so perhaps this property could be named "expiresAt".
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.
Modified expiryDate to expiresAt as suggested to be in sync with rest of the date fields.
"SystemData": { | ||
"type": "object", | ||
"description": "The system data", | ||
"readOnly": true, |
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.
readOnly: true
has no significance for schemas that are only used in responses, so you should remove this.
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.
Removed the readonly:true from SystemData method
"required": true, | ||
"type": "string", | ||
"x-ms-client-name": "ApiVersion", | ||
"x-ms-parameter-location": "method" |
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 x-ms-parameter-location: method
for api-version? This typically goes on the client.
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.
Modified x-ms-parameter-location to client instead of method.
1) Changed workflowRunId parameter to policyId 2) Changed expiryDate to expiresAt 3) ApiVerion changed from method to get from client.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
1) Make "principal" and "principalGroup" an array rather than a single value to be consistent with Devops policies. 2) Remove System.string from policyId to make it language agnostic.
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.
This looks good for preview. 👍
Data Plane API - Pull Request
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous Open API document (swagger) if applicable, and the root paths that have been updated.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links