Add GetCredentialName() #4428
Merged
Add GetCredentialName() #4428
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
antkmsft
requested review from
schaabs,
ahsonkhan,
RickWinter,
vhvb1989,
gearama and
LarryOsterman
as code owners
RickWinter
reviewed
Mar 10, 2023
LarryOsterman
requested changes
Mar 13, 2023
sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp
Outdated
Show resolved
Hide resolved
sdk/identity/azure-identity/inc/azure/identity/chained_token_credential.hpp
Show resolved
Hide resolved
sdk/identity/azure-identity/src/client_certificate_credential.cpp
Outdated
Show resolved
Hide resolved
ahsonkhan
reviewed
Mar 13, 2023
sdk/identity/azure-identity/test/ut/chained_token_credential_test.cpp
Outdated
Show resolved
Hide resolved
…onstructed name; Explicit tests for GetCredentialName()
antkmsft
commented
Mar 14, 2023
antkmsft
commented
Mar 14, 2023
sdk/identity/azure-identity/test/ut/client_secret_credential_test.cpp
Outdated
Show resolved
Hide resolved
antkmsft
commented
Mar 14, 2023
sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp
Outdated
Show resolved
Hide resolved
LarryOsterman
approved these changes
Mar 14, 2023
There was a problem hiding this comment.
In general, I'm not a fan of global functions which are scoped to a specific class implementation, I much prefer making class membership explicit and not scoping class specific methods to the source file implementing the class.
I'm also somewhat troubled by the number of places where you have credName as a parameter to individual credentials (specifically the ManagedIdentity related changes) - but I understand a desire to hide implementation details from customers.
…when calling GetCredentialName()
…-sdk-for-cpp into tokencred-getname
LarryOsterman
added a commit
that referenced
this pull request
Mar 30, 2023
* Add GetCredentialName() (#4428) * Add GetCredentialName() * Update * Undo accidental change * Clang-format * Call GetCredentialName() instead of using constant; Return in-place constructed name; Explicit tests for GetCredentialName() * PR feedback * constructor parameter + non-virtual GetCredentialName() * Update sdk/core/azure-core/CMakeLists.txt * Update sdk/identity/azure-identity/test/ut/client_secret_credential_test.cpp * Update sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp * GCC and Clang warnings * Promote ThrowIfNotSafeCmdLineInput() to private member; avoid copies when calling GetCredentialName() * Spelling * Fix deprecated usage * Fix iteration * Clang-format --------- Co-authored-by: Anton Kolesnyk <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 5702 (#4453) * add healthinsights to Test-SampleMetadata script * spacing * update productSlug to azure-health-insights --------- Co-authored-by: Asaf Levi <[email protected]> * Use aka.ms link to Identity troubleshooting (#4449) * Use aka.ms link to Identity troubleshooting * Update default_azure_credential.cpp * Update default_azure_credential.cpp --------- Co-authored-by: Anton Kolesnyk <[email protected]> * Undocument ChainedCred usage by DefaultAzCred & remove friend and private ctor (#4447) * Undocument ChainedCred usage by DefaultAzCred & remove friend and private ctor * Clang warning fix --------- Co-authored-by: Anton Kolesnyk <[email protected]> * Sync eng/common directory with azure-sdk-tools for PR 5691 (#4450) * typespec renaming * add back scripts for cadl * support .cadl and .tsp * rename * add newline at the end of file --------- Co-authored-by: FAREAST\chunyu <[email protected]> * fix cspell for readme.ms in libcurl sterss test (#4441) * fix cspell * capitalize Valgrind and ubuntu * sdada * fix2 * Simpler identity logging (#4455) * Simpler identity logging * Even simpler * Remove refactoring artifact * Cosmetic change * foreach --------- Co-authored-by: Anton Kolesnyk <[email protected]> * 3rd Party Libs in Samples (#4408) Adding guidance on the proper usage of 3rd Party Libraries in our samples. * Update changelog with issue link (#4458) Co-authored-by: Anton Kolesnyk <[email protected]> * Tests: replace most `EXPECT_TRUE(a OP b)` with `EXPECT_OP(a, b)` (#4457) * Tests: replace most `EXPECT_TRUE(a OP b)` with `EXPECT_OP(a, b)` * Undo unnecessary change --------- Co-authored-by: Anton Kolesnyk <[email protected]> * Revert "[check-spelling] Temporarily pin Node 18 to 18.13.0 (#5537)" (#4456) This reverts commit 8a02e02adfc0d213509fce2764132afa74bd4ba4. Co-authored-by: Mike Harder <[email protected]> * Fix potentially high CPU usage on Windows (#4448) * Fix potentially high CPU usage on Windows * Undo unnecessary formatting * Undo unnecessary changelog * Undo unnecessary formatting * Undo unnecessary formatting * Uninclude locale * Add issue link to changelog * EXPECT_TRUE(a == b) => EXPECT_EQ(a, b) * Update second changelog with link as well --------- Co-authored-by: Anton Kolesnyk <[email protected]> * Ensure the comparison is unsigned to unsigned (#4464) * Ensure the comparison is unsigned to unsigned * Remove cast * Sync eng/common directory with azure-sdk-tools for PR 5742 (#4465) * add some default output to see about minimizing any occurrence of the task failing for no reason. perhaps having some output will allow devops to have an easier time with the invocation * update message * Update eng/common/scripts/trust-proxy-certificate.ps1 Co-authored-by: Wes Haggard <[email protected]> * Update CODEOWNERS (#4461) Fixup an invalid user * Organize applying Identity log prefix (#4459) * Organize applying Identity log prefix * logLevel * Cosmetic changes --------- Co-authored-by: Anton Kolesnyk <[email protected]> * Explicitly set PSNativeCommandArgumentPassing to Legacy for git push script (#4481) https://learn.microsoft.com/en-us/powershell/scripting/learn/experimental-features?view=powershell-7.3#psnativecommandargumentpassing Do to that breaking change in PS 7.3 we need to opt into the legacy arg parsing. Co-authored-by: Wes Haggard <[email protected]> * Fix unmatched parenthesis in doc (#4482) * fix concurrent upload failures (#4484) * Sync eng/common directory with azure-sdk-tools for PR 5726 (#4488) * rerun flag * rerun failed stress test * naming & commenting * update * function and var renaming for better readability * readability & exit on error --------- Co-authored-by: Albert Cheng <[email protected]> --------- Co-authored-by: Anton Kolesnyk <[email protected]> Co-authored-by: Anton Kolesnyk <[email protected]> Co-authored-by: Azure SDK Bot <[email protected]> Co-authored-by: Asaf Levi <[email protected]> Co-authored-by: FAREAST\chunyu <[email protected]> Co-authored-by: George Arama <[email protected]> Co-authored-by: Ronnie Geraghty <[email protected]> Co-authored-by: Mike Harder <[email protected]> Co-authored-by: Rick Winter <[email protected]> Co-authored-by: Wes Haggard <[email protected]> Co-authored-by: Wes Haggard <[email protected]> Co-authored-by: JinmingHu <[email protected]> Co-authored-by: Albert Cheng <[email protected]>
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4418.
Note that this PR is not about getting rid of ChainedTokenCredential's special the private special constructor for DefaultAzureCredential - that will likely be taken care of in a separate subsequent PR, and would depend on what we decide for #4419. This PR has only 1 out of 2 changes needed to remove that constructor.