-
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
[Unified Recorder] New test-utils
package for test credential
#19423
[Unified Recorder] New test-utils
package for test credential
#19423
Conversation
Co-authored-by: Zachary Foster <[email protected]>
Rename function to match the credential being used
…to harshan/issue/recorder/19387
… harshan/issue/recorder/19387
test-utils
package for test credential
"packageName": "@azure-tools/identity-extensions", | ||
"projectFolder": "sdk/test-utils/identity-extensions", | ||
"versionPolicyName": "utility" | ||
}, |
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.
Moving fast 🔥
|
||
import { ClientSecretCredential } from "@azure/identity"; | ||
import { env, isPlaybackMode } from "@azure-tools/test-recorder"; | ||
import { NoOpCredential } from "@azure-tools/test-recorder-new"; |
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.
Shouldn’t NoOpCredential be moved to this package?
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.
Hmm, can be done
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.
Very nice to see this quickly.
"@azure/core-auth": "^1.3.2", | ||
"@azure-tools/test-recorder": "^1.0.0", | ||
"@azure-tools/test-recorder-new": "1.0.0", | ||
"@azure/identity": "^2.0.1" |
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.
Identity currently uses the recorder. Wouldn’t it be an issue?
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.
identity depends on
- recorder
test-utils/identity-extensions depends on
- recorder
- identity
every other SDK will depend on
- recorder
- identity
- test-utils/identity-extensions
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.
identity doesn't depend on test-utils/identity-extensions, so there should not be a problem
Co-authored-by: Will Temple <[email protected]>
Co-authored-by: Will Temple <[email protected]>
Co-authored-by: Will Temple <[email protected]>
Co-authored-by: Will Temple <[email protected]>
Co-authored-by: Will Temple <[email protected]>
…shaNalluru/azure-sdk-for-js into harshan/issue/recorder/19387
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
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.
Let’s go! 🚀
* - returns the ClientSecretCredential (expects AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET in your environment or in the .env file) | ||
* - AAD traffic won't be recorded if this credential is used. | ||
*/ | ||
export function createTestCredential() { |
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.
please take an optional TokenCredentialOptions
parameter.
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.
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.
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 haven't seen the need to use the persistence option, but @sadasant can advise here.
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 would leave it as TokenCredentialOptions to avoid guaranteeing that this API uses CSC in the back end. I am optimistic that we can eventually make this work with DefaultAzureCredential, even if we have to plumb something through the karma request pipeline.
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.
Looks good!
|
modify servicefabricmesh parameters (Azure#19423) Co-authored-by: ZiWei Chen (WICRESOFT NORTH AMERICA LTD) <[email protected]>
Provides the credential to be used in the tests.
NoOpCredential
in playback andClientSecretCredential
in record/live modes.Fixes #19387