Release purview microsoft.purview preview/2024 04 01 preview #28792
Conversation
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge. |
Swagger Validation Report
|
| Compared specs (v0.10.8) | new version | base version |
|---|---|---|
| purview.json | 2024-04-01-preview(ecf736f) | 2021-12-01(main) |
| purview.json | 2024-04-01-preview(ecf736f) | 2023-05-01-preview(main) |
The following breaking changes are detected by comparison with the latest stable version:
Only 30 items are listed, please refer to log for more details.
The following breaking changes are detected by comparison with the latest preview version:
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️LintDiff: 1 Warnings warning [Detail]
| Compared specs (v2.2.2) | new version | base version |
|---|---|---|
| package-2024-04-01-preview | package-2024-04-01-preview(ecf736f) | default(main) |
[must fix]The following errors/warnings are introduced by current PR:
| Rule | Message | Related RPC [For API reviewers] |
|---|---|---|
| Booleans properties are not descriptive in all cases and can make them to use, evaluate whether is makes sense to keep the property as boolean or turn it into an enum. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1657 |
The following errors/warnings exist before current PR submission:
Only 30 items are listed, please refer to log for more details.
| Rule | Message |
|---|---|
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L211 |
|
Long-running delete operations must have responses with 202, 204 and default return codes. They also must have no other response codes. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L222 |
|
A 202 response should include an Location response header. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L250 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L259 |
|
Properties of a PATCH request body must not have default value, property:managedEventHubState. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L301 |
|
Properties of a PATCH request body must not have default value, property:managedResourcesPublicNetworkAccess. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L301 |
|
Properties of a PATCH request body must not have default value, property:publicNetworkAccess. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L301 |
|
A 202 response should include an Location response header. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L313 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L322 |
|
x-ms-pageable extension must be specified for LIST APIs.Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L436 |
|
Usage of Guid is not recommended. If GUIDs are absolutely required in your service, please get sign off from the Azure API review board. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L453 |
|
Get endpoints for collections of resources must only have the value and nextLink properties in their model.Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L483 |
|
Synchronous POST operations must have one of the following combinations of responses - 200 and default ; 204 and default. They also must not have other response codes. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L502 |
|
scopeTenantId is a query parameter. Post operation must not contain any query parameter other than api-version. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L512 |
|
scopeType is a query parameter. Post operation must not contain any query parameter other than api-version. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L512 |
|
scope is a query parameter. Post operation must not contain any query parameter other than api-version. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L512 |
|
Usage of Guid is not recommended. If GUIDs are absolutely required in your service, please get sign off from the Azure API review board. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L519 |
|
Operations API path must follow the schema provided in the common types. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1097 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1283 |
|
Long-running delete operations must have responses with 202, 204 and default return codes. They also must have no other response codes. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1294 |
|
A 202 response should include an Location response header. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1329 |
|
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1338 |
|
x-ms-pageable extension must be specified for LIST APIs.Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1505 |
|
Definitions must not have properties named additionalProperties except for user defined tags or predefined references. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L1950 |
|
The parameter 'subscriptionId' is defined in global parameters section without 'x-ms-parameter-location' extension. This would add the parameter as the client property. Please ensure that this is exactly you want. If so, apply the extension 'x-ms-parameter-location': 'client'. Else, apply the extension 'x-ms-parameter-location': 'method'. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L3008 |
|
The parameter 'api-version' is defined in global parameters section without 'x-ms-parameter-location' extension. This would add the parameter as the client property. Please ensure that this is exactly you want. If so, apply the extension 'x-ms-parameter-location': 'client'. Else, apply the extension 'x-ms-parameter-location': 'method'. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L3015 |
| Not using the common-types defined parameter 'scope'. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L472 |
|
| The summary and description values should not be same. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L502 |
|
| OperationId should contain the verb: 'removedefaultaccount' in:'DefaultAccounts_Remove'. Consider updating the operationId Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L508 |
|
| Not using the common-types defined parameter 'scope'. Location: Microsoft.Purview/preview/2024-04-01-preview/purview.json#L538 |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView 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.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
|
Generated ApiView
|
AzureRestAPISpecReview
added
ARMChangesRequested
|
Please address or respond to feedback from the ARM API reviewer. |
JeffreyRichter
added
the
BreakingChange-Approved-UserImpact
rismittal
removed
the
ARMChangesRequested
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
|
Regarding the LintDiff warning about boolean vs enum, I believe for the proposed field, a boolean should work as it is an irreversible state and only intended to be true or false. It is also scoped inside of the containing object as that is the only scenario it is intended to be set in and is for a specific use case and not a generalized use case. |
| "tenantEndpointState": { | ||
| "description": "Gets or sets the state of tenant endpoint.", | ||
| "enum": [ | ||
| "NotSpecified", |
There was a problem hiding this comment.
I am curious about whether you intend to allow setting it to the NotSpecified state, and whether that is what it defaults to - you might want to update the description to clarify
There was a problem hiding this comment.
Now that I see it, I also feel a concern that you allow setting publicNetworkAccess to NotSpecified and that I'm not familiar with any other RPs doing that for private links Is that a 'new' invention from your RP?
Or am I just out of date with my knowledge of what network team recommended for this?
There was a problem hiding this comment.
“Not specified” is for our internal logic to differentiate between existing accounts vs newly created accounts. But once the call is received, it’ll be set to enabled/disabled based on the internal checks.
There was a problem hiding this comment.
So, do I understand correctly that you would never actually want 'NotSpecified' to be sent in the request, and never want to return it in the response?
If I got that right, it seems quite confusing to add it to the enumeration!
Or do you mean it could be returned, but only for older objects?
How would people know what the effective behavior is, in that case?
There was a problem hiding this comment.
- If the value is explicitly given as 'Disabled'/'Enabled' - we honor it.
- If it is 'NotSpecified' -
- If it is a new account creation after the feature launch- it will be enabled by default.
- If it is updating an exisiting account - it'll be disabled by default till the customer performs mandatory setup before enabling it.
- If a value is continued to be 'NotSpecified'- it helps us differentiate that its an existing account that did not get updated after the feature launch.
There was a problem hiding this comment.
@ddumesnil-microsoft
But why do you need a 'NotSpecified' value at all, in json format? Why not just have the field be modeled as optional?
Since you seem to say it won't return NotSpecified in the response. It will return Disabled or Enabled, is that right?
There was a problem hiding this comment.
NotSpecified could be returned in the response. It is the value that is used for accounts that existed before the feature launched (as new accounts are Enabled by default and could otherwise be updated to Disabled) and have not undergone any updates (as that would set it to Disabled by default).
This allows us to have easier tracked values as the value will be present for all accounts rather than using null as the NotSpecified value.
...ification/purview/resource-manager/Microsoft.Purview/preview/2024-04-01-preview/purview.json
Show resolved
Hide resolved
...ification/purview/resource-manager/Microsoft.Purview/preview/2024-04-01-preview/purview.json
Outdated
Show resolved
Hide resolved
| }, | ||
| "deprovisioned": { | ||
| "description": "The deprovisioned status of the account.\r\nOnly applicable for the secondary account.", | ||
| "type": "boolean", |
There was a problem hiding this comment.
Did you see the warning about how booleans are not recommended because they're not extensible? They also tend to lead to you having to combine many booleans, in order to express more states, when potentially one enum could have done the job. #Resolved
There was a problem hiding this comment.
Yes, I did see the warning. For this case, a boolean should work as it is an irreversible state and only intended to be true or false. It is also scoped inside of the containing object as that is the only scenario it is intended to be set in and is for a specific use case and not a generalized use case.
There was a problem hiding this comment.
I get that you're thinking of deprovisioning as a one-time irreversible thing, but is it really something that happens independently and orthogonally of all the other state transitions your resource can go through? Modeling it as a bool could make it hard for people to understand the combined state machine, if there are dependency rules. Just raising all my doubts for you to consider.
There was a problem hiding this comment.
Yes, I totally understand your concerns, but in this specific case, it is intended to be one-time and irreversible. This field is merely an indicator of the account state and is intended to be transitional as well. Through this API, we will use this field to display some UX messages to a user. It is similar to a deleted state and is a one-way operation.
| "description": "The public Account Merge Info model.", | ||
| "type": "object", | ||
| "properties": { | ||
| "accountLocation": { |
There was a problem hiding this comment.
[ARMBlocking] how are you going to do RBAC permissions check on this? #Resolved
There was a problem hiding this comment.
This is purely for reporting and informational purposes. Calls will not be performed on the account listed inside of the Account Merge Info.
To overshare, this object contains information from us merging two Purview accounts data. The account location, name, resource group, and subscription id properties are for us to know which account this account merged with. It is solely for informational purposes.
There was a problem hiding this comment.
But how, for instance, do you avoid spoofing, or information disclosure?
There was a problem hiding this comment.
(BTW thanks for clarifying that these properties are mostly readonly, maybe that answers the concern?)
There was a problem hiding this comment.
Yes, through this API, they will only be read only. This information is only exposed to users within the same tenant where they would also have knowledge of this account information. I don't believe there should be any concerns with these fields as they are only for information. So when an account is loaded after a merge, the UX can display which account it was merged to.
...ification/purview/resource-manager/Microsoft.Purview/preview/2024-04-01-preview/purview.json
Show resolved
Hide resolved
| @@ -2616,7 +2697,8 @@ | |||
| "properties": { | |||
| "$ref": "#/definitions/PrivateLinkResourceProperties", | |||
| "description": "The private link resource properties.", | |||
| "readOnly": true | |||
| "readOnly": true, | |||
There was a problem hiding this comment.
It seems like you're changing your private links related definitions. Would this be a good opportunity to refactor to using common-types privatelinks.json?
Which can help ensure API consistency, and easier reviewing going forward?
Any blockers to doing so that you've identified?
There was a problem hiding this comment.
I looked into the common-types and have a few questions that could potentially present as blockers. There are some fields that seem to misalign from what we store in our repository and the common-types definitions.
For example, common-types defines PrivateEndpointConnection as a Resource, but we represent it as a ProxyResource. Similarly, the group-ids field in PrivateEndpointConnectionProperties is not present in our repo (likely because we are only storing the information as a proxy and connection, not the full resource) so that also misaligns.
What is the advice here in terms of refactoring? My concern on switching to the common-types is we may lose this nuance in our system (e.g. ProxyResource) and have data that is never actually returned by our system since we do not store it anywhere (e.g. group-ids).
There was a problem hiding this comment.
I think technically ProxyResource should be correct, and they are both similar - both definitions should not include things that are specific to TrackedResource. I don't think switching between them is likely to cause any issues, and the automated breaking changes validations that run on PRs here should hopefully catch any issues.
I am not sure whether group-ids is one of those parts of the private link contract that is not universally the same between RPs. I'd suggest we try to clarify that with the owners of the contract.
As long as the property is optional, and not required, I think technically its fine not to return it - see, for instance the way nextLink is always declared, but not actually always in responses. Adding it to your documented properties may look like a breaking change, and we can run that past breaking change reviewers to see what they think about the issue, but if you never actually return the property, or never return it except for new api-versions, I don't see how adding it to your documented set of optional properties is going to ever break anybody in practice.
I think it would still be an SDK breaking change, but hopefully if so it is at least one with some small upsides - overall helping better structure and maintain SDKs - and you're wanting to make some changes to private links to fix quality issues in your definitions anyway right? Including one enum change: PrivateEndpointConnectionStatus
Would breaking change experts like to add more guidance on whether its better or not to refactor at this point?
There was a problem hiding this comment.
PS my general understanding, is that group id(s) should be specified by your service since it used when creating private endpoint connection(s) to explain the logical 'group' of endpoints which this private endpoint connection is to connect to. For instance, some services might have multiple hosted-on-behalf FQDNs that the customer can use, to connect to the service, and choose to split those up into functional groups for specific purposes the customer wishes to enable, e.g. (data ingestion, service management, persistence, monitoring).
I can see that maybe your API seems to be fine without providing this kind of information, but I also feel that in principle all private endpoint connections should provide this information. And ideally, it shouldn't actually be a breaking change, to start returning it, in a new api version.
But anyway, I think you should probably either stick with what you're doing, and return nothing - or actually start returning something.
There was a problem hiding this comment.
I will add this into the task to also investigate the addition of group ids for private endpoints. Thanks for the information Tim!
TimLovellSmith
added
the
ARMChangesRequested
openapi-workflow-bot
bot
removed
the
WaitForARMFeedback
ddumesnil-microsoft
removed
the
ARMChangesRequested
openapi-workflow-bot
bot
added
the
WaitForARMFeedback
FWIW I think adding x-ms-flatten is sdk breaking, and not really considered 'better practice' these days. I don't think it was really worth it just to fix linter warnings, in case that is the only reason its being added Refers to: specification/purview/resource-manager/Microsoft.Purview/preview/2024-04-01-preview/purview.json:2701 in ecf736f. [](commit_id = ecf736f, deletion_comment = False) |
|
Adding a new optional property to an API contract is not breaking; however, it still requires a new service api-version so customers can know what api-version supports the new property. |
There was a problem hiding this comment.
For your schema definitions, please reference the common-types schema definitions from (some version of) privatelinks.json instead of adding your own. If you face any blockers with that please let me know.
e.g.
"200": {
"description": "OK - Returns the specified of private link resource",
"schema": {
"$ref": "../../../../../common-types/resource-management/v6/privatelinks.json#/definitions/PrivateLinkResource"
}
},
rkmanda
added
the
ARMSignedOff
|
Please add a workitem to refactor the private links related definitions. |
rkmanda
removed
the
WaitForARMFeedback
|
@rkmanda here is the task to update to common-types for Private Links: Task 3198663: Update Private Links/Connections/Endpoints to use Common Types for GA. |
...ification/purview/resource-manager/Microsoft.Purview/preview/2024-04-01-preview/purview.json
Show resolved
Hide resolved
| @@ -2892,18 +2973,17 @@ | |||
| "description": "List of usage information", | |||
| "type": "object", | |||
| "properties": { | |||
| "nextLink": { | |||
| "description": "The Url of next link.", | |||
| "type": "string" | |||
There was a problem hiding this comment.
Sometimes people remember to annotate these as "format": "uri". I'm not sure how much it matters, TBH.
|
/pr RequestMerge |
…8792) * Initial commit for adding 2024-04-01-preview API version * Changes for 2024-04-01-preview API version * PR Feedback
ARM (Control Plane) API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.Note
As of January 2024 there is no PR assignee. This is expected. See https://aka.ms/azsdk/pr-arm-review.
PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
Click here to see the details of Step 1, Breaking Changes review
If you are in purview of Step 1 of the diagram, follow the Breaking Changes review process.
IMPORTANT! This applies even if you believe your PR was mislabeled, for any reason, including tool failure.
Click here to see the details of Step 2, ARM review
See https://aka.ms/azsdk/pr-arm-review.
Click here to see the diagram footnotes
Diagram footnotes
[1] See ARM review queue (for PR merge queues, see [2]).
[2] public repo merge queue, private repo merge queue (for ARM review queue, [1])
The ARM reviewer on-call engineer visits the merge queue twice a day, so the approximate ETA for merges is 12 - 24 hours.
Purpose of this PR
What's the purpose of this PR? Check the specific option that applies. This is mandatory!
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.
Additional information
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.
Getting help
Purpose of this PRandDue diligence checklist.Next Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.