-
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
[Hub Generated] Review request for Microsoft.HealthcareApis to add version stable/2023-11-01 #26628
[Hub Generated] Review request for Microsoft.HealthcareApis to add version stable/2023-11-01 #26628
Conversation
Automatic PR validation restarted. This comment will be populated with next steps to merge this PR once validation is completed. Please wait ⌛. |
Swagger Validation Report
|
compared swaggers (via Oad v0.10.4)] | new version | base version |
---|---|---|
healthcare-apis.json | 2023-11-01(9577d77) | 2023-09-06(main) |
healthcare-apis.json | 2023-11-01(9577d77) | 2022-10-01-preview(main) |
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 tags (via openapi-validator v2.1.6) | new version | base version |
---|---|---|
package-2023-11 | package-2023-11(9577d77) | default(main) |
[must fix]The following errors/warnings are introduced by current PR:
Rule | Message | Related RPC [For API reviewers] |
---|---|---|
Consider using x-ms-client-flatten to provide a better end user experience Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L4085 |
The following errors/warnings exist before current PR submission:
Only 30 items are listed, please refer to log for more details.
Rule | Message |
---|---|
ResourceNameRestriction |
The resource name parameter 'resourceName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L37 |
LroErrorContent |
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L138 |
PatchResponseCodes |
Long-running PATCH operations must have responses with 200, 202 and default return codes. They also must not have other response codes. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L143 |
PatchIdentityProperty |
The patch operation body parameter schema should contain property 'identity'. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L168 |
LroPatch202 |
The async patch operation should return 202. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L178 |
LroErrorContent |
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L188 |
LroLocationHeader |
A 202 response should include an Location response header. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L220 |
LroErrorContent |
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L229 |
ResourceNameRestriction |
The resource name parameter 'resourceName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L361 |
GetCollectionOnlyHasValueAndNextLink |
Get endpoints for collections of resources must only have the value and nextLink properties in their model.Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L385 |
ResourceNameRestriction |
The resource name parameter 'resourceName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L406 |
ResourceNameRestriction |
The resource name parameter 'privateEndpointConnectionName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L406 |
ProvisioningStateSpecifiedForLROPut |
201 response schema in long running PUT operation is missing ProvisioningState property. A LRO PUT operations response schema must have ProvisioningState specified for the 200 and 201 status codes. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L450 |
PutRequestResponseSchemeArm |
A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'PrivateEndpointConnections_CreateOrUpdate' Request Model: 'parameters[5].schema' Response Model: 'responses[200].schema' Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L450 |
PutResponseCodes |
Synchronous and long-running PUT operations must have responses with 200, 201 and default return codes. They also must not have other response codes. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L450 |
LroErrorContent |
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L492 |
DeleteResponseCodes |
Long-running delete operations must have responses with 202, 204 and default return codes. They also must have no other response codes. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L503 |
LroLocationHeader |
A 202 response should include an Location response header. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L530 |
LroErrorContent |
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L539 |
ResourceNameRestriction |
The resource name parameter 'resourceName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L551 |
GetCollectionOnlyHasValueAndNextLink |
Get endpoints for collections of resources must only have the value and nextLink properties in their model.Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L575 |
ResourceNameRestriction |
The resource name parameter 'resourceName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L593 |
ResourceNameRestriction |
The resource name parameter 'groupName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L593 |
ResourceNameRestriction |
The resource name parameter 'workspaceName' should be defined with a 'pattern' restriction. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L723 |
PutResponseCodes |
Synchronous and long-running PUT operations must have responses with 200, 201 and default return codes. They also must not have other response codes. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L764 |
LroLocationHeader |
A 202 response should include an Location response header. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L812 |
LroErrorContent |
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L821 |
LroLocationHeader |
A 202 response should include an Location response header. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L868 |
LroErrorContent |
Error response content of long running operations must follow the error schema provided in the common types v2 and above. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L877 |
DeleteResponseCodes |
Long-running delete operations must have responses with 202, 204 and default return codes. They also must have no other response codes. Location: Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json#L882 |
️️✔️
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.
️️✔️
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).
️️✔️
PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️
Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
|
Generated ApiView
|
/pr RequestMerge |
This is a nonblocking feedback but you might want to do a scrub of description docs - sometimes you're capitalizing Dicom, other times you're not. Also as a general programmer, I don't know what DICOM means, so maybe it would make sense to unabbreviate it. Refers to: specification/healthcareapis/resource-manager/Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json:3440 in 9577d77. [](commit_id = 9577d77, deletion_comment = False) |
Again you might want to check docs for consistent capitalization of Fhir/FHIR etc Refers to: specification/healthcareapis/resource-manager/Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json:3811 in 9577d77. [](commit_id = 9577d77, deletion_comment = False) |
I'd like to understand the differences between this definition of encryption settings, and others like Do you not support using a managed identity to access the encryption key? Or is the idea that which identity to use would be implicit? Refers to: specification/healthcareapis/resource-manager/Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json:4081 in 9577d77. [](commit_id = 9577d77, deletion_comment = False) |
It seems like your resource type supports both system assigned and user assigned managed identity, does it not support use of both for CMK? In reply to: 1810509262 Refers to: specification/healthcareapis/resource-manager/Microsoft.HealthcareApis/stable/2023-11-01/healthcare-apis.json:4081 in 13d6f29. [](commit_id = 13d6f29, deletion_comment = False) |
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 address or respond to feedback from the ARM API reviewer. |
Removed ARMSignedOff because new commits came in. Adding ARMChangesRequested to reflect the open question on the encryption settings. |
Yes, we follow these guidelines for the schema - https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/common-api-contracts.md#customer-managed-key-encryption But leave out We also do not support cross-tenant CMK yet, so do not need support with the |
I think at a minimum its probably a good idea to add the 'keyEncryptionKeyIdentity' property as supported, with all possible enum values, so that you can show callers using this API version the type of identity set with future api versions that support more identity kinds. Even if you don't actually support setting it to anything but the default, or setting the other identity kinds for now. |
Wondering if this is a hard requirement or a suggestion? We unfortunately were not able to get a review out sooner because we had a previous API version, 2023-09-06, that didn't get its swagger checked in until last week, and was unable to get anyone to review a draft / previous PRs which did not branch off of main. Now we are at a point where adding keyEncryptionKeyIdentity for 2023-11-01 would be very difficult, especially given the holiday freeze. And supporting multiple identities is not something in the plan to support for either service, so its a hard sell for something we may or may not do in a couple of years |
/pr RequestMerge |
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.
Approving for now for ARM, with the noted warning about future-proofness.
Swagger pipeline restarted successfully, please wait for status update in this comment. |
This is a PR generated at OpenAPI Hub. You can view your work branch via this link.
ARM (Control Plane) API Specification Update Pull Request
PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
[1] ARM review queue (for merge queues, see [4])
The PRs are processed by time opened, ascending. Your PR may show up on 2nd or later page.
If you addressed Step 1 from the diagram and your PR is not showing up in the queue, ensure the label
ARMChangesRequested
is removed from your PR. This should cause the label
WaitForARMFeedback
to be added.[2] https://aka.ms/azsdk/support/specreview-channel
[3] List of SDK breaking changes approvers in pinned Teams announcement
[4] public repo merge queue, private repo merge queue (for ARM review queue, [1])
If you need further help with anything, see
Getting help
section below.Purpose of this PR
What's the purpose of this PR? Check all that apply. 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 Step 2, "ARM Review", for this PR.
Breaking changes review (Step 1)
you must follow the breaking changes process.
IMPORTANT This applies even if:
Such claims must be reviewed, and the process is the same.
ARM API changes review (Step 2)
ARMReview
label.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 ApiView
comment 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
Swagger-Suppression-Process
to get approval.
Getting help
and https://aka.ms/ci-fix.