[keyvault-secrets] Migrate to TypeSpec #31848
Conversation
|
API change check API changes are not detected in this pull request. |
| @@ -0,0 +1,86 @@ | |||
| #!/usr/bin/env node | |||
|
|
|||
| const { execSync } = require("child_process"); | |||
There was a problem hiding this comment.
The end result is that this file should be deleted once all the workarounds are no longer needed
There was a problem hiding this comment.
Should we have this at the top level? Or should we have one per package (in case there are package-specific things which need to be done, generating from a specific package's spec, etc)?
There was a problem hiding this comment.
Yeah, I can move it inside the package folder. My hope (and so far that's been the case) is that the same set of customizations need to happen for all packages. I figured if they start diverging I can push them down into the individual package folder. But my real hope is that this script gets deleted by end of January as its a workaround, not a solution.
There was a problem hiding this comment.
Yeah if there are no differences between packages no problem with just keeping the script here. Whatever is least work for you.
| ) | ||
| .replace( | ||
| /nbf: !item\["notBefore"\] \? item\["notBefore"\] : item\["notBefore"\].getTime\(\),/g, | ||
| 'nbf: !item["notBefore"] ? item["notBefore"] : item["notBefore"].getTime() / 1000,' |
There was a problem hiding this comment.
May want to truncate the decimal here
| } | ||
| } | ||
| return mapPagedAsyncIterable( | ||
| this.client.getSecretVersions(secretName, options), |
There was a problem hiding this comment.
Random thought, do we know if maxPageSize works here? It's not defined on PageOptions in the generated code so am wondering if the property is now being ignored.
Cc @MaryGao
There was a problem hiding this comment.
maxPageSize is no longer support as a general option and we update the guideline also here.
@deyaaeldeen @xirzec I was wondering if we need to provide a standard way to expose this parameter for services still needing this option: https://github.com/search?q=repo%3AAzure%2Fazure-sdk-for-js+maxPageSize+language%3ATypeScript+samples-dev&type=code.
There was a problem hiding this comment.
In the mean time for Key Vault perhaps we can customize the paging helpers to accept maxPageSize and pass it as maxresults like Key Vault is expecting? I'm not sure what kind of effort would be needed for that
There was a problem hiding this comment.
Yeah, let me investigate this here: Azure/autorest.typescript#2802.
There was a problem hiding this comment.
@maorleger we discussed this internally and I could work with for paginate helper to support maxPageSize if breaking is not acceptable.
There was a problem hiding this comment.
Thank you, I replied in the issue Azure/autorest.typescript#2802 (comment) and think we want to continue supporting maxPageSize in keyvault 🙏
| @@ -0,0 +1,86 @@ | |||
| #!/usr/bin/env node | |||
|
|
|||
| const { execSync } = require("child_process"); | |||
There was a problem hiding this comment.
Should we have this at the top level? Or should we have one per package (in case there are package-specific things which need to be done, generating from a specific package's spec, etc)?
|
|
||
| // The authentication policy must come after the deserialization policy since the deserialization policy | ||
| // converts 401 responses to an Error, and we don't want to deal with that. | ||
| this.client.pipeline.removePolicy({ name: bearerTokenAuthenticationPolicyName }); | ||
| this.client.pipeline.addPolicy(keyVaultAuthenticationPolicy(credential, pipelineOptions), { |
There was a problem hiding this comment.
Same comment as certs, we don't need to place this after deserializationPolicy anymore since it no longer exists
maorleger
force-pushed
the
keyvault-secrets-typespec
branch
2 times, most recently
from
05d2065 to
98658f7
Compare
maorleger
force-pushed
the
keyvault-secrets-typespec
branch
from
98658f7 to
8c01e6f
Compare
-### Packages impacted by this PR
@azure/keyvault-secrets
Issues associated with this PR
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR: (Applicable only to SDK release request PRs)
Checklists