-
Notifications
You must be signed in to change notification settings - Fork 528
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
feat(daemon): lazy image saving #1121
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1121 +/- ##
==========================================
- Coverage 75.16% 75.06% -0.11%
==========================================
Files 108 108
Lines 7724 7800 +76
==========================================
+ Hits 5806 5855 +49
- Misses 1363 1380 +17
- Partials 555 565 +10
Continue to review full report at Codecov.
|
pkg/v1/daemon/image.go
Outdated
if err := i.initialize(); err != nil { | ||
return nil, err | ||
} | ||
return i.tarballImage.LayerByDigest(h) |
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.
Should be LayerByDiffID
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.
Thanks. Fixed bb5ea5e
LGTM in general other than the one bug. I would like to see a test (you should be able to catch most issues by calling |
@jonjohnsonjr Thanks for the review! I fixed the bug and added |
Thanks! |
Fix #627
A faster way of getting an image ID is really useful for caching. We don't want to call
docker save
if we already have the image information in the cache. For example, in our case, we should skip scanning vulnerabilities of the image when the scan result exists in the cache. The cache key is an image ID, so it is enough to calldocker inspect
to know only the image ID.This implementation is based on
computed()
as below.go-containerregistry/pkg/v1/mutate/image.go
Line 51 in 45aaa6c
The downside is that
daemon.Image
doesn't return an error and it might return an error when each method is called (knqyf263@881e16e).I probably should add tests, but I'd like to hear your thought on this idea before working on tests 🙇
I also want to replace
ConfigFile
with this approach in another PR because I think we can generate the config file from the result ofdocker inspect
anddocker history
.