[Key Vault Admin] Convenience layer - KeyVaultBackupClient #11009
Conversation
ghost
added
the
| * @param sasToken The SAS token. | ||
| * @param [options] The optional parameters. | ||
| */ | ||
| public async beginBackup( |
There was a problem hiding this comment.
There was a problem hiding this comment.
cc: @christothes , @heaths
There was a problem hiding this comment.
.NET guidelines specify start* but I don't see any specific guidance around naming for the other langs.
There was a problem hiding this comment.
In JS we favor "begin". Assuming that this prefix is fine, what I'm most concerned is of the full part.
There was a problem hiding this comment.
I don't have a strong opinion here, but I opted for plain backup because there is no other backup operation to contrast with.
| sasToken: string; | ||
| requestOptions?: BeginBackupOptions; | ||
| intervalInMs?: number; | ||
| resumeFrom?: string; |
There was a problem hiding this comment.
this is more of a question, but I thought I saw a continuation_token property in some of your other poller options (or was that paging options)
There was a problem hiding this comment.
The continuation token is part of the internal implementation. It's hidden from the public API here, but it's definitely there!
| * ```ts | ||
| * const client = new KeyVaultBackupClient(url, credentials); | ||
| * | ||
| * const blobStorageUri = "<blob-storage-uri>"; // <Blob storage URL>/<folder name> |
There was a problem hiding this comment.
In this case the folder name is a separate argument.
| * const blobStorageUri = "<blob-storage-uri>"; // <Blob storage URL>/<folder name> | |
| * const blobStorageUri = "<blob-storage-uri>"; |
There was a problem hiding this comment.
@christothes I thought the same, but my tests pass with the same parameters I'm using in the others, which satisfy: <Blob storage URL>/<folder name>. Do you have different parameters for these tests?
Co-authored-by: Christopher Scott <chriscott@hotmail.com>
| /** | ||
| * Status of the restore operation. | ||
| */ | ||
| status?: string; |
There was a problem hiding this comment.
doesn't this always have some kind of known status, even if it's like "started" or something?
There was a problem hiding this comment.
Also it would be nice if this was a string union, but maybe we don't have that kind of safety?
There was a problem hiding this comment.
This property comes from the service! And, the swagger doesn't have a set of possible values :/ I could ask though. What do you think after this information?
There was a problem hiding this comment.
Note: this property is not part of core-lro, it's part of the service we're using for this client.
|
|
||
| let response: KeyVaultClientFullRestoreOperationResponse; | ||
| try { | ||
| response = await client.fullRestoreOperation(vaultUrl, setParentSpan(span, options)); |
There was a problem hiding this comment.
optional nit: you could do the assignment and return inside the try block if you wanted:
try {
const response = await client.fullRestoreOperation(vaultUrl, setParentSpan(span, options));
return response;
} finally {
span.end();
}There was a problem hiding this comment.
Does that still allow the span.end() to run? 😮 I can also just return client.full*
There was a problem hiding this comment.
returns inside a try still execute the finally, but you'll want to do an await or else the time spent inside the promise won't be tracked by the span
|
@bterlson thank you! |
The Backup Client source code.
IMPORTANT:
getOperationStateto show the other values of the pending operation. This gets extended by the properties of the generated code's operation.You can see the tests here: #11010
This PR will fail until these PRs are merged:
[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient [Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815Merged.[Key Vault Admin] No LROs on the generated code for now [Key Vault Admin] No LROs on the generated code for now #11008Merged.For: #10799