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

use k8s keychain first #1346

Merged
merged 1 commit into from
Apr 13, 2022
Merged

use k8s keychain first #1346

merged 1 commit into from
Apr 13, 2022

Conversation

dprotaso
Copy link
Contributor

This speeds up instances where the credentials are in image pull secrets

This speeds up instances where the credentials are in image pull secrets
Comment on lines +49 to 50
k8s,
authn.DefaultKeychain,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
k8s,
authn.DefaultKeychain,
authn.DefaultKeychain,
k8s,

I think we may still want the default keychain first, and it should at least be much quicker than the others, barring any cred helper shenanigans.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok with that .

Just be aware that if you're testing k8schain (pulling from creds from secrets) and you're hitting a real cluster you'll want to docker logout beforehand :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

nvm, I think I'm becoming convinced that k8s belongs first, since it eliminates the issue with slow cred helpers and potential confusion with local testing. Let's keep k8s first. (Sorry for the review churn)

@dprotaso
Copy link
Contributor Author

ok k8s is first now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants