Add KeyClient.GetCryptographyClient #23856
Conversation
heaths
requested review from
KrzysztofCwalina,
schaabs and
tg-msft
as code owners
ghost
added
the
|
Out of curiosity, what motivates this update? Is it just the guidelines change, or is there a customer scenario that calls for it? If the latter, it might be worth adding something to samples to show users how to call the method, and indicate when/why they'd want to do this instead of newing a |
|
Just for the guidelines change. It may also improve the discoverability. I can add (or modify) a sample, though. |
| internal static Uri CreateKeyUri(Uri vaultUri, string name, string version) => version is null | ||
| ? new Uri(vaultUri, KeysPath + name) | ||
| : new Uri(vaultUri, $"{KeysPath}{name}/{version}"); |
There was a problem hiding this comment.
Can we avoid adding this method by calling keyClient.GetKey(name, version) and then using the returned key to create the crypto client? (That's how I was thinking to implement this for C++) .
There was a problem hiding this comment.
Not sure about dotnet or C++, but just chatted with JS architects and in JS we would not have an async get<Subclient> method and it would need to be async if we want to call keyClient.GetKey
There was a problem hiding this comment.
Sometimes I forget that C++ is the only lang with all APIs being Sync. So there's not such restriction for C++ to consider the Async world.
There was a problem hiding this comment.
The method you highlighted just creates a URI, which your CryptographyClient should already take in its public constructor. We also already have methods that get you a KeyVaultKey, from which you can get a JsonWebKey, which users should be able to pass to a CryptographyClient already.
There's no reason to have an async version of any methods I added because methods are sync or async, not classes. That may not be true of Python and Java, however, which have separate classes but might choose to have 1 method on each that returns the same "syncedness" client (that's what I'd do, but talk with your architects).
|
Adding "Do Not Merge" until @KrzysztofCwalina, @tg-msft, or @schaabs can review. |
Resolves #23786