[Identity] Migrated to samples v2#19085
Conversation
ghost
added
the
witemple-msft
left a comment
witemple-msft
left a comment
There was a problem hiding this comment.
I always only thought of dependencyOverrides as being to override dependency version selectors and not to inject them, but this should totally work and is honestly quite clever.
I have one comment about how the build errors might be fixed, and another about why we need to import from the identity package name and not from ../src.
| import { ClientSecretCredential } from "../src"; | ||
| import { KeyClient } from "@azure/keyvault-keys"; |
There was a problem hiding this comment.
Two comments here:
- We need to still be importing from
@azure/identityhere. Adding it to thepathsentry in tsconfig should enable this, so you don't need to import from source for that to resolve (as you can think ofpathsas adding a manual module resolution entry to the TypeScript compiler). - We will also need an entry for
@azure/keyvault-keys. I never thought to usedependencyOverridesfor this purpose, but it should totally work if we also add apathsentry for it! I think something like"@azure/keyvault-keys": ["../../keyvault/keyvault-keys/src/index"]should work nicely.
There was a problem hiding this comment.
So, I updated the references to @azure/identity on the samples, but "@azure/keyvault-keys": ["../../keyvault/keyvault-keys/src/index"] didn’t work. We talked over teams and we realized that adding keyvault on paths would alter significantly how the dist folder appears (it grabs a large part of the repository structure, to group these two projects under the same root).
and a cleanup related to keys on the package.json
sadasant
requested review from
deyaaeldeen,
mikeharder,
praveenkuttappan and
schaabs
as code owners
| - [API Reference Documentation](https://docs.microsoft.com/javascript/api/@azure/identity) | ||
| - [Product documentation](https://azure.microsoft.com/services/active-directory/) | ||
| - [Samples](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/samples) | ||
| - [Samples](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/samples/v2) |
There was a problem hiding this comment.
I think other versions could be added in the future so do we want the customer to pick and choose the right version for them?
There was a problem hiding this comment.
Ohh interesting! — Though I’d rather not think too much about it at the moment 🙏 I took a peek and it made me question other things. Makes me feel it’s a scope leak and could lead to a rabbit hole.
There was a problem hiding this comment.
I took a peek and it made me question other things.
@sadasant interesting, anything you could share that could help with future migrations?
| // Licensed under the MIT license. | ||
|
|
||
| /** | ||
| * @summary Authenticates with a client and a client's secret. |
There was a problem hiding this comment.
The word client is overloaded in this context and could be confusing. Should we use "application" instead?
There was a problem hiding this comment.
Updated! Please let me know how it looks 👍
witemple-msft
left a comment
witemple-msft
left a comment
There was a problem hiding this comment.
Very nice. I have just a couple of comments on the @summary tags.
| // Licensed under the MIT license. | ||
|
|
||
| /** | ||
| * @summary Authenticates with a client and a client's secret. |
Co-authored-by: Will Temple <witemple@microsoft.com>
| // Licensed under the MIT license. | ||
|
|
||
| /** | ||
| * @summary Authenticates with a client and a app registration's secret. |
There was a problem hiding this comment.
could you update this summary as well?
There was a problem hiding this comment.
Oh now I think I should have said Authenticates with an app registration’s client Id and secret. I’ll update it!
3 participants
I’m following the migration guide to move Identity to the samples v2: https://github.com/Azure/azure-sdk-for-js/wiki/Samples-v2-Migration-Guide
Important: Added the
dependencyOverridesin order to require@azure/keyvault-keyson the samples, since Identity can’t depend on@azure/keyvault-keys(it would get recursive).Fixes #14474