Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[KeyVault] - Ensure keyOps is used when importing a key #18295

Closed
wants to merge 3 commits into from
Closed

[KeyVault] - Ensure keyOps is used when importing a key #18295

wants to merge 3 commits into from

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Oct 20, 2021 •
edited
Loading

What

  • Use swagger transformations to ensure key_ops is not camelized
  • Prefer key_ops, but use keyOps in importKey if necessary
  • add dom.d.ts file instead of inlining the declaration

Why

I originally thought that the JsonWebKey type is used only on output
(deserializing from the service). I forgot about importKey which takes a JWK as
input. In that case, if a user supplied both key_ops and keyOps we would have
taken keyOps as our transformations code wasn't run.

Having fixed the code, I decided a swagger transformation would make more sense
since it allows me to keep backwards compat code (keyOps -> key_ops) instead of
forward compat code (key_ops -> keyOps)...

Resolves #18294

@maorleger maorleger requested a review from sadasant as a code owner October 20, 2021 23:53
@ghost ghost added the KeyVault label Oct 20, 2021
maorleger
maorleger commented Oct 20, 2021
View reviewed changes
sdk/keyvault/keyvault-keys/swagger/README.md
@@ -48,3 +48,22 @@ directive:
$.values[0].value = "Rotate";
$.values[1].value = "Notify";
```

### Prevent key_ops from being converted to keyOps when generating types for JsonWebKey
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarangan12 / @joheredi - any advice for this swagger transform? I originally talked about this with @joheredi offline but now that it's in PR form I'd love to hear any feedback.

Goal is to avoid camel-casing key_ops into keyOps in our generated code.

joheredi
joheredi approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@maorleger maorleger closed this Oct 25, 2021
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request May 6, 2022
@hari-bodicherla @aspand
@kamusta-msft @prchin @kimberlyyipc @D1v38om83r @rane-rajasi @PushyaragY
CodeGen from PR 17988 in Azure/azure-rest-api-specs
6eb8592 
ComputeRP 2022-03-01 REST API release (Azure#17988)

* new API version set up

* update mistakes

* move extended location to common

* update other readme files for sdk languages

* Adding UltraSSDcapability properties in dedicated host resource (Azure#18076)

* Added diffDiskSettings property as part of Swagger changes needed for Ephemeral VM\VMSS

* updated comment

* updated swagger specs for diffdisksettings property

* updated swagger spec comments  for diff disk settings [property

* added example to create Diff OS disk scaleset

* updated 2018-10-01 version specs with diffdisk property

* added example file for creating VM with diffdisksettings property

* updated swagger changes for reimage operation in single vm

* update examples

* udpated examples

* fixed validation errors

* updated comments for reimage operation documentation

* Updated examples and documentation for APIs in swagger

* updated examples as per review comments

* updated swagger documentation

* updated swagger documentation with zone details in the sku example

* updated swagger documentation and reverted the breaking changes

* updated examples as per swagger model

* updated swagger to remove the model validation errors for existing examples where we are passing read only parameter in the request

* updated swagger

* updated swagger

* Added new property in DiffDiskSettings

* updated swagger spec

* udpated swagger

* updated swagger spec

* updated code

* updated code

* udpated swagger

* updated code

* updated swagger documentation for DiffDiskPlacement

* updated code

* updated documentation

* updated code

* updated swagger

* updated swagger

* updated swagger

* updated code

* updated code

* updated example json

* updated swagger changes

* updated swagger

* Updating minor documentation for the changes checked in PR: 18076 (Azure#18528)

* Added diffDiskSettings property as part of Swagger changes needed for Ephemeral VM\VMSS

* updated comment

* updated swagger specs for diffdisksettings property

* updated swagger spec comments  for diff disk settings [property

* added example to create Diff OS disk scaleset

* updated 2018-10-01 version specs with diffdisk property

* added example file for creating VM with diffdisksettings property

* updated swagger changes for reimage operation in single vm

* update examples

* udpated examples

* fixed validation errors

* updated comments for reimage operation documentation

* Updated examples and documentation for APIs in swagger

* updated examples as per review comments

* updated swagger documentation

* updated swagger documentation with zone details in the sku example

* updated swagger documentation and reverted the breaking changes

* updated examples as per swagger model

* updated swagger to remove the model validation errors for existing examples where we are passing read only parameter in the request

* updated swagger

* updated swagger

* Added new property in DiffDiskSettings

* updated swagger spec

* udpated swagger

* updated swagger spec

* updated code

* updated code

* udpated swagger

* updated code

* updated swagger documentation for DiffDiskPlacement

* updated code

* updated documentation

* updated code

* updated swagger

* updated swagger

* updated swagger

* updated code

* updated code

* updated example json

* updated swagger changes

* updated swagger

* udpated swagger

* Merged the intent from origin (Azure#18159)

* fix naming convention error =s

* new API version set up

* update mistakes

* move extended location to common

* update other readme files for sdk languages

* Adding UltraSSDcapability properties in dedicated host resource (Azure#18076)

* Added diffDiskSettings property as part of Swagger changes needed for Ephemeral VM\VMSS

* updated comment

* updated swagger specs for diffdisksettings property

* updated swagger spec comments  for diff disk settings [property

* added example to create Diff OS disk scaleset

* updated 2018-10-01 version specs with diffdisk property

* added example file for creating VM with diffdisksettings property

* updated swagger changes for reimage operation in single vm

* update examples

* udpated examples

* fixed validation errors

* updated comments for reimage operation documentation

* Updated examples and documentation for APIs in swagger

* updated examples as per review comments

* updated swagger documentation

* updated swagger documentation with zone details in the sku example

* updated swagger documentation and reverted the breaking changes

* updated examples as per swagger model

* updated swagger to remove the model validation errors for existing examples where we are passing read only parameter in the request

* updated swagger

* updated swagger

* Added new property in DiffDiskSettings

* updated swagger spec

* udpated swagger

* updated swagger spec

* updated code

* updated code

* udpated swagger

* updated code

* updated swagger documentation for DiffDiskPlacement

* updated code

* updated documentation

* updated code

* updated swagger

* updated swagger

* updated swagger

* updated code

* updated code

* updated example json

* updated swagger changes

* updated swagger

* Updating minor documentation for the changes checked in PR: 18076 (Azure#18528)

* Added diffDiskSettings property as part of Swagger changes needed for Ephemeral VM\VMSS

* updated comment

* updated swagger specs for diffdisksettings property

* updated swagger spec comments  for diff disk settings [property

* added example to create Diff OS disk scaleset

* updated 2018-10-01 version specs with diffdisk property

* added example file for creating VM with diffdisksettings property

* updated swagger changes for reimage operation in single vm

* update examples

* udpated examples

* fixed validation errors

* updated comments for reimage operation documentation

* Updated examples and documentation for APIs in swagger

* updated examples as per review comments

* updated swagger documentation

* updated swagger documentation with zone details in the sku example

* updated swagger documentation and reverted the breaking changes

* updated examples as per swagger model

* updated swagger to remove the model validation errors for existing examples where we are passing read only parameter in the request

* updated swagger

* updated swagger

* Added new property in DiffDiskSettings

* updated swagger spec

* udpated swagger

* updated swagger spec

* updated code

* updated code

* udpated swagger

* updated code

* updated swagger documentation for DiffDiskPlacement

* updated code

* updated documentation

* updated code

* updated swagger

* updated swagger

* updated swagger

* updated code

* updated code

* updated example json

* updated swagger changes

* updated swagger

* udpated swagger

* Merged the intent from origin (Azure#18159)

* fix naming convention error =s

* fix issues from Azure/azure-rest-api-specs#18159

* Adding Identity to VirtualMachineScaleSetVM (Azure#18295)

* init

* PR comment

* [Crash Consistent RestorePoints] Making consistencyMode input parameter for RestorePoint (Azure#18165)

* removing readonly for consistencymode

* small spell correction 2.1

* [RestorePoints] Fixing instanceView (Azure#18592)

* fixing instance view 20220301instanceviewfix branch 1.1

* adding objects 2.1

* VMSS Flex Disk deleteOption changes (Azure#18433)

* disk delete option for vmss flex

* prettier changes

* update parameters

Co-authored-by: Kimberly Yip Chang <[email protected]>

* Fix vm extension location bug  (Azure#18487)

* fix

* fix lint issue

* Update properties for VMApps, CRP (Azure#18609)

* saving work

* ran prettier

* fix LintDiff, SpellCheck

* reverted changes made to gallery.json. The changes to gallery.json will be part of separate PR.

* GuestPatching: Adding AutomaticByPlatformSettings (in VMSS and VM model) and useRollingUpgradePolicy (in VMSS model) properties (Azure#18581)

* GuestPatching: Adding AutomaticByPlatofrmSettings within Linux and Windows Patch settings for VM and VMSS model

* Updating examples related to AutomaticByPlatformSettings property in PatchSettings for VM and VMSS model

* Adding useRollingUpgradePolicy property to automaticOSUpgradePolicy for VMSS and other minor changes for AutomaticByPlatfrom settings

* Addressing PR feedback

* Addressing PR feedback #2

* Renaming reboot reference for windows and linux automaticbyplatform patch settings - reboot settings

* Resolving errors reported on the PR by Avocado and LintDiff

* add architecture added in last version

* fix CI failures

* added PremiumV2_LRS (Azure#18809)

* Update dedicatedHost.json

* fix example name to keep it consistent with others

* Update computeRPCommon.json

Co-authored-by: hari-bodicherla <[email protected]>
Co-authored-by: aspand <[email protected]>
Co-authored-by: kamusta-msft <[email protected]>
Co-authored-by: prchin <[email protected]>
Co-authored-by: Kimberly Yip Chang <[email protected]>
Co-authored-by: Kimberly Yip Chang <[email protected]>
Co-authored-by: Bhaskar Brahma <[email protected]>
Co-authored-by: Rajasi Rane <[email protected]>
Co-authored-by: PushyaragY <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joheredi joheredi joheredi approved these changes

@sadasant sadasant Awaiting requested review from sadasant

@sarangan12 sarangan12 Awaiting requested review from sarangan12

Assignees
No one assigned
Labels
KeyVault
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[KeyVault] - importKey will currently ignore key_ops
2 participants
@maorleger @joheredi