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

Bump acr cred helper dep #1247

Merged
merged 2 commits into from Jan 20, 2022
Merged

Bump acr cred helper dep #1247

merged 2 commits into from Jan 20, 2022

Conversation

imjasonh
Copy link
Collaborator

@imjasonh imjasonh commented Jan 19, 2022

This picks up chrismellard/docker-credential-acr-env#2 to fix hanging behavior when checking to see whether the ACR cred helper could be used to auth against a registry.

cd pkg/authn/k8schain
go get -u github.com/chrismellard/docker-credential-acr-env@master
go mod tidy && go mod download
cd ../../../cmd/krane
go get -u github.com/google/go-containerregistry/pkg/authn/k8schain@main
go mod tidy && go mod download
cd ../../
./hack/presubmit.sh # passes

This picks up
chrismellard/docker-credential-acr-env#2 to fix
hanging behavior when checking to see whether the ACR cred helper could
be used to auth against a registry.
@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2022

Codecov Report

Merging #1247 (b5af5f5) into main (890d5b3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1247   +/-   ##
=======================================
  Coverage   73.77%   73.77%           
=======================================
  Files         111      111           
  Lines        8285     8285           
=======================================
  Hits         6112     6112           
  Misses       1571     1571           
  Partials      602      602           

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 890d5b3...b5af5f5. Read the comment docs.

@dprotaso
Copy link
Contributor

dprotaso commented Jan 19, 2022

/lgtm
cc @jonjohnsonjr @mattmoor

@@ -10,49 +10,49 @@ replace (

require (
github.com/google/go-containerregistry v0.8.1-0.20220110151055-a61fd0a8e2bb
github.com/google/go-containerregistry/pkg/authn/k8schain v0.0.0-20211223213658-2874338840a6
github.com/google/go-containerregistry/pkg/authn/k8schain v0.0.0-20220114205711-890d5b362eb8
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you'll need to update this in a separate PR

Copy link
Contributor

@dprotaso dprotaso Jan 19, 2022

Choose a reason for hiding this comment

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

Actually - probably not worth doing this here until k8s chain update merges

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure we need to do this in another PR, since it's just replaced above. If downstream deps who don't respect the replace are broken by this (I think that's what you're describing?), we should figure out a way to capture that in a presubmit to prevent future breakages.

In any case, I can go get -u again after this merges. I'm kinda hating all these interdependent modules in this repo 🤦 .

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case, I can go get -u again after this merges.

Oh nm - I didn't see the replace directive. That's not necessary then. It would be If you want krane to be go install-able.

I'm kinda hating all these interdependent modules in this repo 🤦 .

Yeah the explicit refs are not fun - I'll open a go issue ;)

imjasonh added a commit to imjasonh/pipeline that referenced this pull request Jan 19, 2022
@imjasonh imjasonh mentioned this pull request Jan 19, 2022
5 tasks
@mattmoor mattmoor merged commit ac864e5 into google:main Jan 20, 2022
@hasheddan
Copy link
Contributor

Thank you @imjasonh @dprotaso 🙌🏻

hasheddan added a commit to hasheddan/crossplane that referenced this pull request Jan 20, 2022
This update includes the fix in the ACR (Azure) cred helper to honor
timeouts and skip attempting to acquire credentials if the registry URL
is not ACR/MCR.

ACR Helper Fix: chrismellard/docker-credential-acr-env#2
K8schain Fix: google/go-containerregistry#1247

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
hasheddan added a commit to hasheddan/crossplane that referenced this pull request Jan 21, 2022
This update includes the fix in the ACR (Azure) cred helper to honor
timeouts and skip attempting to acquire credentials if the registry URL
is not ACR/MCR.

ACR Helper Fix: chrismellard/docker-credential-acr-env#2
K8schain Fix: google/go-containerregistry#1247

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
(cherry picked from commit 76f69fd)
hasheddan added a commit to hasheddan/crossplane that referenced this pull request Jan 21, 2022
This update includes the fix in the ACR (Azure) cred helper to honor
timeouts and skip attempting to acquire credentials if the registry URL
is not ACR/MCR.

ACR Helper Fix: chrismellard/docker-credential-acr-env#2
K8schain Fix: google/go-containerregistry#1247

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
(cherry picked from commit 76f69fd)
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

5 participants