-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Identity] Add VSCodeCredential #7994
Conversation
if (scopeString.indexOf("offline_access") < 0) { | ||
scopeString += " offline_access"; | ||
} | ||
let refreshToken = await keytar.findPassword("VS Code Azure"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should try to avoid reading this every time getToken is called. Do you know what the performance of it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VS Code Azure
should be a const with a descriptive name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance-wise it's similar to Azure SDK credentials, perhaps a bit faster for the initial connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We should add some tests for this though. If not in this PR we should file an issue to track adding the coverage.
@schaabs - sounds good, I'll file an issue for that. |
PR Azure#7994 introduced a build break to rollup by referencing `process` inside `DefaultAzureCredential`. This fix adds a browser version of `DefaultAzureCredential` without the reference to `process`.
PR #7994 introduced a build break to rollup by referencing `process` inside `DefaultAzureCredential`. This fix adds a browser version of `DefaultAzureCredential` without the reference to `process`.
PR Azure#7994 also introduced a problem where a new dependency `keytar` is not bundle safe and needed to be excluded in test bundles that referenced it indirectly via @azure/identity.
PR #7994 also introduced a problem where a new dependency `keytar` is not bundle safe and needed to be excluded in test bundles that referenced it indirectly via @azure/identity.
This adds the VSCodeCredential and adds it to the DefaultAzureCredential.
Drafting for feedback before I finish.
This PR also adds the user-defined managed identity to the DefaultAzureCredential.