Align the casing of LifetimeActionsType with the service. #24475
Conversation
|
Hi, @adarce 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] |
Swagger Generation Artifacts
|
ghost
added
the
Generated ApiView
|
AzureRestAPISpecReview
added
BreakingChangeReviewRequired
|
Hi @adarce, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. |
The change to revert the service back to using "Notify"/"Rotate" instead of lowercase has been merged. The next deployment will probably begin early next week, and it takes 5 business days to complete. |
|
@JeffreyRichter @mikekistler this is aligning the swagger to match the service - or at least what the service did do and will soon again. Can you approve? |
mikekistler
added
the
Approved-BreakingChange
Updated. |
@jlichwa is that actually what CP returns though? In a previous conversation, you indicated that DP was likely changed to align with CP. |
I checked CP has lower case. We aligned service to Swagger specs which all had lower case, as we now change service back to PascalCase- I want it to be consistent everywhere, so swagger needs to be updated as well as service change rolled back. I believe @adarce created separate PR for CP |
CP's implementation does not attempt to perform any normalization for casing.
|
Great so just the swagger update on CP - in general it is ok to accept anything, the only thing return value has to be normalized to swagger to align with REST API guidelines for DP. |
|
CP should not be changed. It's a breaking change. They may be inconsistent, but that is how the service behavior shipped and should be retained. Only the swaggers should be updated. CP is still case sensitive - only case-insensitive URLs are allowed. /cc @JeffreyRichter |
Swagger Generation Artifacts
|
|
@heaths @JeffreyRichter @mikekistler unfortunately having the same constant defined in different casings as here is going to cause us issues: To workaround API inconsistencies we're now normalizing whatever value the API returns to the value defined in the API Definition, for example: To do that, we're outputting a map of the possible values, with the key being the value lower-cased, and then iterating over those until we find a match - which would lead to a compile time issue for us here since these 4 values (or rather, 2 sets of the same value in different casings) get output into the same map/dictionary, which isn't valid e.g.: The intention from our side there is to allow our side to provide case-sensitive validation for the Create/Update payloads and normalize the values from API Responses to ensure this matches the casing - with the intention of trying to workaround casing inconsistencies within the API. Whilst we can look to update that functionality to use a different approach, the same issue remains of which casing should be used - which I'm assuming is why the linter error called out in this comment exists? However, ultimately even outside of our use-case, I believe is going to cause a considerable amount of user confusion/lost time for consumers of the SDK/API. For example, consider that you're typing in an IDE/editor and autocomplete suggests the value I'm assuming that both Key Vault and Managed HSM have likely started out from the same set of endpoints/payloads and may even be the same codebase with conditionals behind the scenes - however as there's sufficient differences between these now which are are causing confusion (since these are only described in the documentation), and are also now causing issues on the API Definitions side (e.g. this issue) - is it worth splitting the API Definitions for Key Vault and Managed HSMs into separate folders to remove these issues? Whilst I appreciate that would be more work for the Service Team to maintain, it'd better reflect the behaviour of the API today given the differences in these two APIs. Alternatively, per this comment from @jlichwa:
Rather than having SDK Consumers have to determine which casing is relevant here - given the ARM API is proxying the call for the Data Plane API, and thus presumably has context of what it's talking to (e.g. the However unfortunately in it's current state this is going to cause us issues - as such is it possible for this PR to be reverted temporarily for the moment until the issues described above are addressed? |
|
@tombuildsstuff the swagger now matches what the service has been doing for the past couple years. So code either wasn't impacted before, or already normalized for this. The SDKs will normalize the value, but any other REST callers would've already compensated. Given how long the services were returning these values, we made the least bad decision to fix the swaggers and have the differently cased enum values. |
|
@heaths I'd also like to raise that the duplicate enum value left in here has broken us downstream. It appears that the old value of |
Original goal was to properly document "Rotate" that Key Vault returned and "rotate" that Managed HSM returned (and was originally in the swagger), but that case-insensitively duplicative value caused issues with some code generators. Reverts parts of Azure#24475
|
@tombuildsstuff @danielrbradley after internal discussions, we appreciate that having both Generally, though, we recommend case-sensitive comparisons for data plane APIs, per our guidelines linked above. This is what our Azure SDKs do normally. In this particular case, however, we are already updating our code to do case-insensitive comparisons - which was always the plan and the swagger change had nothing to do with that. |
Awesome, that's great thanks for that 👍
That's great to hear - out of interest is that change being applied to the entire Key Vault API? We've noted that the Access Policies API is particularly case-sensitive, insofar as the exact casing has to be specified when removing an Access Policy - so I'm wondering if that API is included in the insensitivity changes? Thanks! |
|
The Azure SDK APIs will only be comparing the key rotation lifetime action enum case-insensitively. All other enum comparisons are case-sensitive per guidelines. As for the rest of the Key Vault REST API, I can't say. The service team indicated that at least key rotation lifetime actions are case-insensitive on the service back end, but I don't know if others are. Given our guidelines I've linked to above, I would assume they are not and stick to what documentation says unless you notice the docs (based on OpenAPI) are wrong - in which case we'd appreciate a bug to fix the docs so they are accurate. :) |
* Remove redundant "rotate" from Keys API Original goal was to properly document "Rotate" that Key Vault returned and "rotate" that Managed HSM returned (and was originally in the swagger), but that case-insensitively duplicative value caused issues with some code generators. Reverts parts of #24475 * Remove now-unnecessary exemptions * Fix description on Notify
* Remove redundant "rotate" from Keys API Original goal was to properly document "Rotate" that Key Vault returned and "rotate" that Managed HSM returned (and was originally in the swagger), but that case-insensitively duplicative value caused issues with some code generators. Reverts parts of #24475 * Remove now-unnecessary exemptions * Fix description on Notify
Data Plane (and Control Plane) API - Pull Request
Azure Key Vault's data plane has been using PascalCase for the LifetimeActionsType enum values, while the REST API spec incorrectly specifies the casing to be camelCase. This PR aligns the REST API spec (of both data plane and control plane) with the service.
Note that this is not a breaking change in the service (which has been using PascalCase since GA).
API Info: The Basics
Is this review for (select one):