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

keyvault: fixing the duplicate constant types #21137

Closed
wants to merge 1 commit into from
Closed

keyvault: fixing the duplicate constant types #21137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.3/certificates.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,7 +1630,7 @@
"AutoRenew"
],
"x-ms-enum": {
"name": "ActionType",
"name": "CertificateActionType",
Copy link
Member

@heaths heaths Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is a breaking change for SDKs but shouldn't be necessary. This name only affects SDKs, which should be referencing individual swaggers to generate separate SDKs for keys, secrets, certificates, and administration (basically, everything else). So conflicts between the two are irrelevant unless you're generating a single SDK targeting all swaggers, though.

That begs the question, though: have you considered using our Go SDKs? https://github.com/Azure/azure-sdk-for-go/tree/main/sdk/keyvault

Alternatively, you could use an autorest transform you can pass to autorest to rename these e.g.:

directive:
- from: certificates.json
  where: $.definitions.Actions
  transform: $.properties.action_type["x-ms-enum"].name = "CertificateActionType";
- from: keys.json
  where: $.definitions.LifetimeActionsType
  transform: $.properties.type["x-ms-enum"].name = "LifetimeActionType";

Copy link

@aristosvo aristosvo Oct 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

$.definitions.Actions should be $.definitions.Action

  - from: certificates.json
    where: $.definitions.Action
    transform: $.properties.action_type["x-ms-enum"].name = "CertificateActionType";
  - from: keys.json
    where: $.definitions.LifetimeActionsType
    transform: $.properties.type["x-ms-enum"].name = "LifetimeActionType";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heaths thanks for the suggestion.

This is a breaking change for SDKs but shouldn't be necessary. This name only affects SDKs, which should be referencing individual swaggers to generate separate SDKs for keys, secrets, certificates, and administration (basically, everything else). So conflicts between the two are irrelevant unless you're generating a single SDK targeting all swaggers, though.

From our side we're trying to generate the SDK using the same approach that Track1 is using today, namely keeping this in a single package - so I think the autorest transform is probably fine as a workaround here, but it begs the question of why not fix the data rather than applying a workaround?

From a logical deployment perspective, since these operations are exposed in the same Swagger directory, I'd expect them to be deployed together - so I'd argue that if these are intended to be separate SDKs then these should be split out (which would also remove the need for this workaround). Whilst this may seem like a pedantic point, fixing the data (either by merging this PR as-is, or by splitting these files into different directories) seems preferable to applying autorest workarounds, which should presumably only be a short-term solution and not widely used, else the source data isn't accurate?

That begs the question, though: have you considered using our Go SDKs? https://github.com/Azure/azure-sdk-for-go/tree/main/sdk/keyvault

Unfortunately we're unable to use the Track2 SDK's for reasons we've raised with the Azure SDK team previously - instead we're using hashicorp/go-azure-sdk which is generated from hashicorp/pandora (for Resource Manager) - and we'll be generating some Data Plane packages into another repository until we look into importing/generating those.

Copy link
Member

@heaths heaths Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There are a lot of services that don't use swagger in non-standard ways and we've been generating our SDKs with these definitions for several years. It was a lot of extra work for almost every SDK language to trim support for endpoints that weren't supposed to be in an SDK, like keys and certs endpoints in the secrets SDK.

Going forward with Cadl - a new description language that can generate both server- and client-side code (and swaggers, for legacy support) we'll create subdirectories per SDK, so these files won't be in the same directory. Ever since we refactored them, they were never meant to represent a single service.

Why can't you use the track 2? I've talked with several teams in the past and showed them our migration guides e.g., https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/keyvault/Azure.Security.KeyVault.Secrets/MigrationGuide.md. Everything should be supported 1:1 between track 1 and track 2 SDKs already except for SAS (not recommended anyway, which is why we didn't generate an SDK).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heaths

It was a lot of extra work for almost every SDK language to trim support for endpoints that weren't supposed to be in an SDK, like keys and certs endpoints in the secrets SDK.

Potentially an odd take, but why would these be stripped out? If the Swagger documents them the SDK's should be generating them.

Going forward with Cadl - a new description language that can generate both server- and client-side code (and swaggers, for legacy support) we'll create subdirectories per SDK, so these files won't be in the same directory. Ever since we refactored them, they were never meant to represent a single service.

Do you have any more context on Cadl / the timeframe there?

Why can't you use the track 2?

We've previously given copious amounts of feedback to the Azure SDK Team as to why we're unable to use the Track2 Go SDK - and since we didn't hear back from the Azure SDK Team about our concerns - ultimately ended up having to build hashicorp/go-azure-sdk.

The hashicorp/go-azure-sdk repository both fixes the concerns we had (albeit using the Track1 base layer as a temporary stop-gap, until we're fully off the Azure SDK for Go) and enables us to embed logic within the Go SDK to both make it more consistent (for example, all operations take a Resource ID object where a Resource is being mutated) - which ultimately reduces the amount of code needed to interact with the SDK.

By comparison, adopting Track2 would mean:

  • increasing the amount of code needed (both per-operation and in general)
  • more admin overhead with nested Go modules (e.g. which semver relates to which API version? a large number of breaking changes every time we update a module)
  • re-introducing the pain of composite Swaggers (which comes back to more admin overhead/which API version is actually being used)
  • hitting up against more Swagger inconsistencies (since hashicorp/go-azure-sdk corrects a bunch of these)

As such whilst Track2 may work great for folks using a small number of packages, unfortunately the design/architectural decisions mean it doesn't work for us since we're vendoring large amounts of the SDK (and are concerned about which API versions we're using) - as such this isn't something we're planning to adopt (even if the amount of code required to use Track2 was reduced in the same manner as hashicorp/go-azure-sdk).

Whilst that's unfortunate, in practice not using the Track2 SDK has enabled us to look into additional functionality (for example, Terraform Resource Generation) that wouldn't have been possible if we used the Track1 or Track2 SDKs (since we're normalizing the Swaggers, for example duck-typing into common types) - so whilst there is an overhead to generating this SDK ourselves, I'd argue it's working out really well for us - and as such unfortunately we have no plans to adopt Track2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heaths I get that you'd like us to use Track2, but I still think you've closed this for the wrong reasons here - @JeffreyRichter any chance you'd be able to take a look here? I think @heaths and I are at an impasse here?

Copy link
Member

Choose a reason for hiding this comment

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

I'll meet with Heath and discuss so I understand the issue and report back here (hopefully next week but it is Thanksgiving).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JeffreyRichter did you have a chance to chat with Heath about this yet?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, Heath & I spoke and as it is SDK breaking, we cannot accept this PR.

Copy link
Contributor Author

@tombuildsstuff tombuildsstuff Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Any chance you could explain how this is SDK breaking? The Breaking Change Action above says this isn't a breaking change:

Screenshot 2022-12-12 at 10 01 44

Screenshot 2022-12-12 at 10 02 30

"modelAsString": false
}
}
Expand Down
2 changes: 1 addition & 1 deletion specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.3/keys.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2200,7 +2200,7 @@
"notify"
],
"x-ms-enum": {
"name": "ActionType",
"name": "LifetimeActionType",
"modelAsString": false,
"values": [
{
Expand Down