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.

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

[azkeys] convert to typespec #23776

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

gracewilcox
Copy link
Member

@gracewilcox gracewilcox commented Nov 19, 2024

Converting keys to generate with typespec

part of #23458

@gracewilcox gracewilcox added KeyVault Client This issue points to a problem in the data-plane of the library. labels Nov 19, 2024
@gracewilcox gracewilcox marked this pull request as ready for review December 3, 2024 23:17
@gracewilcox gracewilcox marked this pull request as draft December 3, 2024 23:23
regexReplace("constants.go", `.*(\bKeyOperationExport\b).*`, "")

// delete strconv
regexReplace("client.go", `\"strconv\"`, "")
Copy link
Member

Choose a reason for hiding this comment

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

If you use goimports, instead of gofmt, then it'll trim out unused imports for you.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm we must have a bug in the emitter. We shouldn't be adding unnecessary imports.

Copy link
Member

Choose a reason for hiding this comment

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

I have had this happen, on occasion, when I remove code (in a transform) that used to call a helper function. Not sure when strconv is used in our generated code.

Copy link
Member

Choose a reason for hiding this comment

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

Good point and that's what's happening here. strconv is used for setting the maxresults header and that's being stripped out via a transform.

Copy link
Member

Choose a reason for hiding this comment

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

If running goimports instead of gofmt obviates this replacement, we should do that instead

@gracewilcox gracewilcox marked this pull request as ready for review January 6, 2025 21:39
}

func main() {
// delete the version path param check (version == "" is legal for Key Vault but indescribable by OpenAPI)
Copy link
Member

Choose a reason for hiding this comment

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

we've left OpenAPI behind, need something like

Suggested change
// delete the version path param check (version == "" is legal for Key Vault but indescribable by OpenAPI)
// delete the version path param check (TypeSpec doesn't allow optional path parameters)

@@ -28,7 +24,7 @@ type DecryptResponse struct {

// DeleteKeyResponse contains the response from method Client.DeleteKey.
type DeleteKeyResponse struct {
// A DeletedKey consisting of a WebKey plus its Attributes and deletion info
// A DeletedKey consisting of a WebKey plus its Attributes and deletion info
Copy link
Member

Choose a reason for hiding this comment

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

Is this extra space in the source?

@@ -18,15 +14,17 @@ type BackupKeyResult struct {

// CreateKeyParameters - The key create parameters.
type CreateKeyParameters struct {
// REQUIRED; The type of key to create.
// REQUIRED; The type of key to create. For valid values, see JsonWebKeyType.
Copy link
Member

Choose a reason for hiding this comment

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

But we renamed JsonWebKeyType to KeyType. Is this blurb added by the emitter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

4 participants