ACR Login: Use Azure SDK library to get the credential and token#3398
ACR Login: Use Azure SDK library to get the credential and token#3398dbalseiro wants to merge 3 commits intoknative:mainfrom
Conversation
|
Welcome @dbalseiro! It looks like this is your first PR to knative/func 🎉 |
|
Hi @dbalseiro. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test Thanks for the PR! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3398 +/- ##
==========================================
+ Coverage 54.50% 54.54% +0.03%
==========================================
Files 173 179 +6
Lines 19694 20260 +566
==========================================
+ Hits 10735 11051 +316
- Misses 7836 8037 +201
- Partials 1123 1172 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lkingland
left a comment
There was a problem hiding this comment.
Looks good! I have one minor improvement worth at least exploring the feasability of (plumbing through a context object) and one concern worth double-checking (that there's no other code depending on creds.ErrCredentialsNotFound being returned).
/approve
/hold pending possible changes per the above
| }, nil | ||
| } | ||
| scope := "https://containerregistry.azure.net/.default" | ||
| token, err := azCredentials.GetToken(context.Background(), policy.TokenRequestOptions{ |
There was a problem hiding this comment.
We might want to plumb through a context object to GetACRCredentialLoader in case this thing can lock up or need canceling.
| if err != nil { | ||
| return oci.Credentials{}, fmt.Errorf("failed to get azure access token: %w", err) | ||
| } | ||
| return oci.Credentials{}, creds.ErrCredentialsNotFound |
There was a problem hiding this comment.
I notice that creds.ErrCredentialsNotFound is no longer returned. This sentinel value may be depended upon by other systems to fall-through (despite using error types as flow-control being an antipattern; it's life). It might be a good idea to double-check that there's no other code depending on exactly that error type to ensure we're not breaking any "error fallback" systems if one of the other generic errors is returned instead.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dbalseiro, lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
/kind bug
Fixes #3288
Release Note