Skip to content

Conversation

@imjasonh
Copy link
Contributor

@imjasonh imjasonh commented Jun 3, 2021

go get -u ./...
go mod vendor
./hack/update-codegen.sh

This updates our containerd dep to v1.5.2, which contains containerd/containerd@363f2c3 to only check CPU arch on-demand the first time it's needed, instead of at init-time.

Fixes #916

@imjasonh imjasonh requested a review from jonjohnsonjr June 3, 2021 16:46
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2021

Codecov Report

Merging #1036 (a7ca1b5) into main (9b2cec9) will not change coverage.
The diff coverage is n/a.

❗ Current head a7ca1b5 differs from pull request most recent head 4923d47. Consider uploading reports for the commit 4923d47 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1036   +/-   ##
=======================================
  Coverage   75.03%   75.03%           
=======================================
  Files         107      107           
  Lines        5072     5072           
=======================================
  Hits         3806     3806           
  Misses        720      720           
  Partials      546      546           

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 9b2cec9...4923d47. Read the comment docs.

@imjasonh
Copy link
Contributor Author

imjasonh commented Jun 3, 2021

Needed to add a lock around defaultKeychain.Resolve to avoid a data race, seemingly newly coming from docker/cli#3025 (Thanks to @jonjohnsonjr for digging that up)

@jonjohnsonjr
Copy link
Collaborator

How would you feel about sync.Onceing the Load?

@imjasonh
Copy link
Contributor Author

imjasonh commented Jun 3, 2021

How would you feel about sync.Onceing the Load?

I'm not opposed in theory, but I might want to make that as a separate change. The mutex is a quick hack to avoid the data race (which seems relatively innocuous anyway).

Skipping the load each time is probably good for performance, especially for short-lived CLI use cases, but something that uses ggcr in a long-running, potentially multi-tenant scenario might unknowingly depend on the load each time, and could be surprised in a bad way if we optimized it.

@imjasonh imjasonh merged commit 162d96e into google:main Jun 3, 2021
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.

Docker version upgrade?

3 participants