-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Ksvijayv/purview metadata policies initial #14938
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
Ksvijayv/purview metadata policies initial #14938
Conversation
Hi, @kshitizvijay 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. [email protected] |
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
Swagger Validation Report
|
Rule | Message |
---|---|
Consider using x-ms-client-flatten to provide a better end user experience Location: Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json#L388 |
|
Consider using x-ms-client-flatten to provide a better end user experience Location: Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json#L499 |
|
OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'MetadataPolicyModel'. Consider using the plural form of 'MetadataPolicy' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. Location: Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json#L59 |
|
OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'MetadataPolicyModel'. Consider using the plural form of 'MetadataPolicy' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. Location: Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json#L110 |
|
OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'MetadataPolicyModel'. Consider using the plural form of 'MetadataPolicy' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change. Location: Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json#L168 |
️️✔️
Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️
ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️
SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️
Cross-Version Breaking Changes succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️
CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️
[Staging] SDK Track2 Validation succeeded [Detail] [Expand]
Validation passes for SDKTrack2Validation
- The following tags are being changed in this PR
️️✔️
[Staging] PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️️✔️
[Staging] SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️
[Staging] Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
Swagger Generation Artifacts
|
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.
Looks like you also are missing an entry in https://github.com/Azure/azure-rest-api-specs/blob/master/specification/purview/data-plane/readme.md
...e.Analytics.Purview.MetadataPolicies/preview/2020-12-01-preview/purviewmetadatapolicies.json
Outdated
Show resolved
Hide resolved
@JeffreyRichter @johanste has this been reviewed by the REST board? |
Hi, @kshitizvijay. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
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 left a number of comments, all minor but important to fix to get the best results from the SDK generators.
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...-plane/Azure.Analytics.Purview.MetadataPolicies/stable/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
Hi @kshitizvijay, Your PR has some issues. Please fix the CI sequentially by following the order of
|
I cannot understand the reason behind the SDK azure-sdk-for-net-track2 failure. |
...plane/Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json
Show resolved
Hide resolved
@jhendrixMSFT Who Can I contact to solve this? |
@kshitizvijay the |
@jhendrixMSFT Why do we run checks that are not required? It seems like this only creates confusion and delay. If this check isn't required, can we remove it from list somehow? Are there other checks here that are not required? |
@nickzhums do you know the answers? |
this repo is maintained by Swagger tooling team. cc @josefree |
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 found a few more small issues. The x-ms-error
- x-ms-error-code
is probably on me ... I think I had the wrong name in my earlier comment / suggested change. Sorry about that.
Outside of these few minor changes, this looks good to me.
"swagger": "2.0", | ||
"info": { | ||
"title": "Purview Metadata Policies Service REST API Document", | ||
"version": "2021-07-01" |
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.
"version": "2021-07-01" | |
"version": "2021-07-01-preview" |
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.
Would this change mean the requests would have "?api-version=2021-07-01-preview" ?
Currently the service only works with 2021-07-01.
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.
Yes -- preview versions of the service should use an api-version that ends in "-preview". If your service does not do that currently then you should leave the version as it is for now, but please use this convention for future previews.
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 service does not implement it right now.
I will leave the version as it is.
...plane/Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
...plane/Azure.Analytics.Purview.MetadataPolicies/preview/2021-07-01/purviewMetadataPolicy.json
Outdated
Show resolved
Hide resolved
], | ||
"type": "object", | ||
"properties": { | ||
"values": { |
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.
@JeffreyRichter @johanste @tg-msft @markweitzel The Azure API Guidelines say that this property should be named "value" rather than "values". Personally I like "values" better since it is an array, so plural feels better to me. Should we allow "value" or "values", or stick to just the one name even though it is a bit clunky?
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.
Personally, I like "value" better too but Azure has always use "value" in the past.
@johanste probably has the most experience with how impactful this change would be, so I'd leave it to him to decide.
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 value
(singular) naming convention comes from OData. I don't think we've been consistent in Azure. I'd do an inventory of what is being used today and unless there is a clear winner/a large majority uses value
, I don't have a strong opinion. We obviously wouldn't want to recommend breaking changes to "fix" the consistency problem, however.
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.
@kshitizvijay We want all Azure services to use value
as the property name for the array in a pageable response. Please update the API to comply with this guidance.
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 will update the API to use value
as the property name for the array in a pageable response.
Any guide on how to introduce this breaking change?
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 need to go through the Breaking Changes Process.
For APIs in preview this is not onerous -- you just have document the breaking change and then keep the prior version functioning for at least 90 days.
* initial commit for purview metadata policies * fixed paths and added fields * fixed issues and added examples for purview metadatapolicy * fixed example file name for purview metadatapolicy * fixed words * fixed model issue * removed readonly property from example * removed readonly proprties from body * fixed response status codes * fixed prettier issue * updated metadata roles API for Purview Metadata policy service * spellcheck and prettier fix * removed skiptoken, count and added autorest extension * sdk changes * fixed not foundbug * removed collection path * removed unreferenced file * fixed error code Co-authored-by: Kshitiz Vijayvargiya <[email protected]>
* initial commit for purview metadata policies * fixed paths and added fields * fixed issues and added examples for purview metadatapolicy * fixed example file name for purview metadatapolicy * fixed words * fixed model issue * removed readonly property from example * removed readonly proprties from body * fixed response status codes * fixed prettier issue * updated metadata roles API for Purview Metadata policy service * spellcheck and prettier fix * removed skiptoken, count and added autorest extension * sdk changes * fixed not foundbug * removed collection path * removed unreferenced file * fixed error code Co-authored-by: Kshitiz Vijayvargiya <[email protected]>
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Please ensure to add changelog with this PR by answering the following questions.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.