Skip to content
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

[dev-tool] Allow samples to use createTestCredential #30535

Closed

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Jul 25, 2024

Packages impacted by this PR

@azure-tools/dev-tool

Issues associated with this PR

Contributes to #30447

Describe the problem that is addressed by this PR

Using AZURE_POD_IDENTITY_AUTHORITY_HOST as a workaround was necessary because we wanted to continue using DefaultAzureCredential in samples but skip Managed Identity when
running samples.

In tests, we use createTestCredential which works well but is not the credential we want to showcase
in sample code.

This PR allows us to use createTestCredential in samples-dev (which will be run by CI) while still promoting DAC
as the credential to use in sample code.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR: (Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description.
  • Does this PR need any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here.)
  • Added a changelog (if necessary).

@github-actions github-actions bot added dev-tool Issues related to the Azure SDK for JS dev-tool KeyVault labels Jul 25, 2024
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@maorleger
Copy link
Member Author

maorleger commented Jul 25, 2024

Regenerated samples for the entire repo and inspecting output

So far so good, but I expect it to no-op in most places (except keyvault-keys which now uses creataeTestCredentail)

@maorleger maorleger marked this pull request as ready for review July 26, 2024 16:26
@@ -18,7 +18,7 @@ export async function main(): Promise<void> {
// This sample uses DefaultAzureCredential, which supports a number of authentication mechanisms.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda weird, and can ben confusing. Maybe we remove that comment anywhere we have it but not sure

Copy link
Member

@timovv timovv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

This is not a strong opinion, and I don't want to block things, but I am having minor second thoughts about the approach. It feels a bit strange to me that the samples in samples-dev/ don't match the output in samples/, in what feels like a significant difference in actual runtime behavior (as opposed to what we were doing already, where although we do transform the samples, the output code does the same thing as the input).

The actual approach of doing the transformation looks good to me, though. If it's not too much effort maybe we could go the other way, and transform from DefaultAzureCredential to createTestCredential on the fly when we are executing the samples in the pipelines? That way, while there will still be a mismatch between the samples that are getting run and what we have checked in, at least the two versions that we have checked in will match.

If you're happy with the current approach though I am more than happy to go ahead with it, I can see arguments either way

@maorleger
Copy link
Member Author

Closing per offline discussion, we prefer to go in another direction so this is obsolete

@maorleger maorleger closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-tool Issues related to the Azure SDK for JS dev-tool KeyVault
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants