-
Notifications
You must be signed in to change notification settings - Fork 544
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
authn/k8schain : fallthrough in case of gcloud
command not available
#405
Conversation
Codecov Report
@@ Coverage Diff @@
## master #405 +/- ##
==========================================
- Coverage 54.18% 54.14% -0.04%
==========================================
Files 87 87
Lines 4108 4111 +3
==========================================
Hits 2226 2226
- Misses 1542 1544 +2
- Partials 340 341 +1
Continue to review full report at Codecov.
|
What does this fix? |
@jonjohnsonjr I've updated the description (should also update the commit message I think) :
|
925bf5a
to
4df552f
Compare
@jonjohnsonjr updated 😉 |
4df552f
to
152a597
Compare
… missing Signed-off-by: Vincent Demeester <[email protected]>
152a597
to
5253b29
Compare
@jonjohnsonjr updated 👼 |
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 but to reiterate pipelines shouldn't be depending on google.Keychain
, removing that code should be safe :)
@jonjohnsonjr yes sir 🎖️ I'll update that on pipeline 😉 |
It's not the prettiest change ever, but it keeps the error in the general case, but allows to fallthrough in a
multikeychain
. This type of errors could be used for other chains too 👼It fixes cases where using
google.Keychain
withauthn.NewMultiKeychain
wheregcloud
command is not available (see https://gist.github.com/vdemeester/c397ffb3fd19b4cc2ba3243dc4db9f83). It should useAnonymous
instead of failing hard in that case.Signed-off-by: Vincent Demeester [email protected]