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

fix(v1/remote): return an error if both auth and keychain are set #1334

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Apr 6, 2022

Fixes #1332

ping @imjasonh

@google-cla
Copy link

google-cla bot commented Apr 6, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@estroz estroz force-pushed the fix/default-keychain-auth branch from 99795ab to 4e03e69 Compare April 6, 2022 20:34
@jonjohnsonjr
Copy link
Collaborator

I haven't verified myself but I'm a little worried about how this might interact with the crane and gcrane packages, which use a keychain my default. This might break callers who supply WithAuth?

@estroz
Copy link
Contributor Author

estroz commented Apr 11, 2022

@jonjohnsonjr I'll investigate and update this PR if I find a reasonable fix.

@estroz
Copy link
Contributor Author

estroz commented Apr 11, 2022

From what I can see there is no way a caller that passes a crane.WithAuth and crane.WithAuthFromKeychain, either explicitly or implicitly via makeOptions, can get this error. Both options override the default keychain option. Effectively, the behavior in remote this PR corrects was already enforced by crane/gcrane.

@codecov-commenter
Copy link

Codecov Report

Merging #1334 (4e03e69) into main (efc62d8) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #1334      +/-   ##
==========================================
- Coverage   74.07%   74.04%   -0.03%     
==========================================
  Files         112      112              
  Lines        8385     8391       +6     
==========================================
+ Hits         6211     6213       +2     
- Misses       1570     1574       +4     
  Partials      604      604              
Impacted Files Coverage Δ
pkg/v1/remote/options.go 71.81% <50.00%> (-2.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efc62d8...4e03e69. Read the comment docs.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

Thanks!

@jonjohnsonjr jonjohnsonjr merged commit 7b4be6b into google:main Apr 13, 2022
@estroz estroz deleted the fix/default-keychain-auth branch April 13, 2022 20:18
digglife added a commit to digglife/cli that referenced this pull request Sep 22, 2022
go-containerregistry now raises error when both keychain and auth are
set(google/go-containerregistry#1334). With this commit, tkn will ignore
keychain when auth is provided with flag.

fixes: tektoncd#1718
tekton-robot pushed a commit to tektoncd/cli that referenced this pull request Sep 28, 2022
go-containerregistry now raises error when both keychain and auth are
set(google/go-containerregistry#1334). With this commit, tkn will ignore
keychain when auth is provided with flag.

fixes: #1718
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.

v1/remote: WithAuth and WithAuthFromKeychain conflict
3 participants