Remove implementation details of key release policy; instead use opaque blob#10916
Remove implementation details of key release policy; instead use opaque blob#10916lmazuel merged 5 commits intoAzure:masterfrom
Conversation
daviddesberg
requested review from
RandalliLama,
heaths and
jlichwa
as code owners
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure CLI Extension Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-java - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-js - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-python - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-net - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-python-track2 - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-resource-manager-schemas - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Trenton Generation - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
| "release_policy": { | ||
| "$ref": "#/definitions/KeyReleasePolicy", | ||
| "description": "The policy rules under which the key can be exported." | ||
| "description": "Blob encoding the policy rules under which the key can be exported.", |
There was a problem hiding this comment.
Should be more specific about supported encodings since I don't see a contentType property like we had discussed over Teams. I.e. either say something like, "Base64-encoded UTF-8 string encoding the policy rules under which the key can be exported." Maybe not the best, but hopefully conveys my concern.
Still, given conversations about versioning, I expected to see a contentType that describes what is in this. Values could look like:
application/jsonapplication/json; charset=utf-8
Either now or in the future, you may also need a version property. While HTTP Content-Type headers don't allow it, you're in control of this property so you could just append something like ; version=2 to the contentType, or just introduce a new property sibling to contentType. Seems better to define that all now so we have the proper support and defaults known and documented now.
| "release_policy": { | ||
| "$ref": "#/definitions/KeyReleasePolicy", | ||
| "description": "The policy rules under which the key can be exported." | ||
| "description": "Blob encoding the policy rules under which the key can be exported.", |
There was a problem hiding this comment.
Given my comment above and that this same definition is repeated many times, I would actually define a data type that should be supported in swagger, but let the validation system check:
"definitions": {
"KeyReleasePolicy": {
"type": "string",
"description": "...",
"format": "base64url"
}Thing is, if you define contentType and/or version, you may want this to still be a model with those three properties and this just becomes an object data type.
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| "contentType": { | ||
| "description": "Content type and version of key release policy", | ||
| "type": "string", | ||
| "default": "application/json; version=1.0" |
There was a problem hiding this comment.
Since you already treat it as UTF8 anyway, it would help inform people if you made this, perhaps:
| "default": "application/json; version=1.0" | |
| "default": "application/json; charset=utf-8; version=1.0" |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@lmazuel Just fixed the remaining validation error under my control. The other ones come up on every AKV Swagger PR. Are any other changes necessary before merging? |
|
/azurepipelines run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
CI are false positive, and @heaths approved, merging |
…ue blob (Azure#10916) * Remove implementation details of key release policy; instead use opaque blob * syntax fix * Include a content-type * add charset * fix export key example
3 participants
Per discussions with @heaths and @herveyw , we are moving SKR policy's implementation out of the Swagger and instead representing it as an opaque blob.
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
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.
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 API Review Board as defined in the Breaking Change Policy.
Please follow the link to find more details on PR review process.