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

[Key Vault Keys] Regenerate swagger using Autorest Typescript v6 #11245

Closed
wants to merge 4 commits into from

Conversation

sadasant
Copy link
Contributor

This PR is related to #9590

Since now the parameters are more strictly defined, the network requests have slightly changed.

Once reviewed, I will proceed to regenerate the recordings and update the other packages.

@sadasant sadasant requested review from sophiajt and xirzec September 15, 2020 00:02
@sadasant sadasant self-assigned this Sep 15, 2020
@ghost ghost added the KeyVault label Sep 15, 2020
kty: keyType,
keySize: options.keySize,
keyOps: options.keyOps,
tags: options.tags
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generated create key accepts curve and key type, however our general options bag in this method doesn't accept "curve". Users can still pass curve to createEcKey. I wonder, should we change the type and accept curve for the main createKey function?

cc: @heaths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got an answer from Heath: createKey should accept curve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a new issue: #11263

const response = await this.client.createKey(this.vaultUrl, name, "EC", options);
const response = await this.client.createKey(this.vaultUrl, name, {
kty: "EC"
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed "options" since by this point options is undefined. See the if conditional above in the code (you should be able to see the } else { part from here).

const response = await this.client.createKey(this.vaultUrl, name, "RSA", options);
const response = await this.client.createKey(this.vaultUrl, name, {
kty: "RSA"
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed "options" since by this point options is undefined. See the if conditional above in the code (you should be able to see the } else { part from here).

@@ -518,7 +544,7 @@ export class KeyClient {

return this.getKeyFromKeyBundle(response);
} else {
const response = await this.client.importKey(this.vaultUrl, name, key, options);
const response = await this.client.importKey(this.vaultUrl, name, { key });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed "options" since by this point options is undefined. See the if conditional above in the code (you should be able to see the } else { part from here).

@@ -620,7 +650,7 @@ export class KeyClient {

return this.getKeyFromKeyBundle(response);
} else {
const response = await this.client.updateKey(this.vaultUrl, name, keyVersion, options);
const response = await this.client.updateKey(this.vaultUrl, name, keyVersion, {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update key doesn't accept an empty options bag, so here we are.

Just as in the other instances, I removed "options" since by this point options is undefined. See the if conditional above in the code (you should be able to see the } else { part from here).

@@ -6,7 +6,7 @@
typescript:
package-name: "@azure/keyvault-keys"
use-extension:
"@microsoft.azure/autorest.typescript": "~5.0.1"
"@autorest/typescript": "6.0.0-dev.20200623.2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this version from the previous PR I did a while ago. Is this the Good Version ™? cc: @joheredi

Copy link
Member

Choose a reason for hiding this comment

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

Can you try with 6.0.0-dev.20200826.1 please? this is the latest one

@sadasant
Copy link
Contributor Author

I'm closing this PR for now. The latest Autorest & Autorest Typescript forces us to use api-version 7.1. It doesn't allow us to specify api-version 7.0, which is a regression. We'll come back to this issue during the next release cycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants