Update Administration backup LROs to conform to guidelines #17440
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
christothes
added
the
Client
christothes
requested review from
KrzysztofCwalina and
tg-msft
as code owners
chamons
reviewed
Dec 17, 2020
There was a problem hiding this comment.
Really dumb question - Keyvault is a shipped service, right?
How is changing public properties names:
public System.Collections.Generic.IList<string> AllowedActions { get { throw null; } }
public System.Collections.Generic.IList<string> AllowedDataActions { get { throw null; } }
public System.Collections.Generic.IList<string> DeniedActions { get { throw null; } }
public System.Collections.Generic.IList<string> DeniedDataActions { get { throw null; } }
not an API break?
|
@chamons , the service shipped, but the SDK with these members hasn't GA'd. We don't always use the same names for a better UX, and that is the case here. |
heaths
approved these changes
Jan 5, 2021
sdk/keyvault/Azure.Security.KeyVault.Administration/src/BackupOperation.cs
Outdated
Show resolved
Hide resolved
| } | ||
| if (_value != null && _value.EndTime.HasValue && _value.Error != null) | ||
| { | ||
| _requestFailedException = new RequestFailedException($"{_value.Error.Message}\nInnerError: {_value.Error.InnerError}\nCode: {_value.Error.Code}"); |
There was a problem hiding this comment.
Do we not already have a utility method in Core, perhaps, that would do this consistently?
There was a problem hiding this comment.
Which part were you thinking should be consistent? The error payload in this case is specific to the service.
sdk/keyvault/Azure.Security.KeyVault.Administration/src/KeyVaultBackupClient.cs
Outdated
Show resolved
Hide resolved
sdk/keyvault/Azure.Security.KeyVault.Administration/src/RestoreOperationInternal.cs
Outdated
Show resolved
Hide resolved
| @@ -37,6 +37,7 @@ public async Task ResumeBackupRestore() | |||
|
|
|||
| // Construct a BackupOperation using a KeyVaultBackupClient and the Id from a previously started operation. | |||
| BackupOperation backupOperation = new BackupOperation(client, backupOperationId); | |||
| /*@@*/backupOperation._retryAfterSeconds = (int)PollingInterval.TotalSeconds; | |||
heaths
approved these changes
Jan 5, 2021
annelo-msft
pushed a commit
to annelo-msft/azure-sdk-for-net
that referenced
this pull request
Feb 17, 2021
* fixup LROs to conform to guidelines, add tests
openapi-sdkautomation bot
pushed a commit
to AzureSDKAutomation/azure-sdk-for-net
that referenced
this pull request
Jan 21, 2022
[Go] Rename stuttering (Azure#17440) * [Go] rename stuttering * fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #17381