-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add manifest merging support #3994
base: master
Are you sure you want to change the base?
Conversation
This one probably can be rebased now (as the other PR was merged) |
This patch reworks and updates the error handling in the docker manifest command to be more inline with modern go principles. Specifically, we use the new errors.Is function, along with errors.Wrap, to preserve error hierarchy instead of using manual string operations. This allows us to simplify various testing components, as well as simplifying the implementation of manifest merging later. Signed-off-by: Justin Chadwell <me@jedevc.com>
Previously, merging multiple manifest lists together was not supported as a use case. However, with new versions of buildkit, manifest lists will be created by default. To support these new manifest lists, we should support merging manifest lists, just as we support this with manifests today. To do this, we refactor the manifest store to save and load groups of manifests, instead of only one. On disk, these manifests take the form of suffixed indexes, e.g. "_2", "_3", etc (note that we ignore "_1", which is permitted to allow a backwards-compatible change with the previous disk format). Then, only the commands need small updates - "create" now looks up multiple manifests and saves multiple manifests, while "annotate" now annotates all the matching manifests. Signed-off-by: Justin Chadwell <me@jedevc.com>
b13aa3d
to
577dc07
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3994 +/- ##
==========================================
- Coverage 59.15% 59.09% -0.06%
==========================================
Files 287 287
Lines 24723 24763 +40
==========================================
+ Hits 14624 14634 +10
- Misses 9214 9236 +22
- Partials 885 893 +8 |
|
||
if !isValidOSArch(imageManifest.Descriptor.Platform.OS, imageManifest.Descriptor.Platform.Architecture) { | ||
return errors.Errorf("manifest entry for image has unsupported os/arch combination: %s/%s", opts.os, opts.arch) | ||
for i, imageManifest := range imageManifests { |
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.
Hm, in hindsight, I'm not convinced about annotate looping through every manifest.
e.g. assuming an image with BuildKit attestations, the platform would be unknown/unknown
, so updating the platform is probably not the right idea?
I guess annotation only makes sense over a single manifest, so when multiple are found, we should error out?
} | ||
|
||
// NotFound interface | ||
func (n *notFoundError) NotFound() {} |
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.
Wondering; instead of a new error-type, could we change the errors to use these, and use errdefs.IsNotFound()
(etc)?
cli/vendor/github.com/docker/docker/errdefs/is.go
Lines 31 to 35 in c173316
// IsNotFound returns if the passed in error is an ErrNotFound | |
func IsNotFound(err error) bool { | |
_, ok := getImplementer(err).(ErrNotFound) | |
return ok | |
} |
- What I did
Introduced support for to merge manifest lists and OCI indexes to the
docker manifest
command.- How I did it
We refactor the manifest store to save and load groups of manifests, instead of only one. On disk, these manifests take the form of suffixed indexes, e.g. "_2", "_3", etc (note that we ignore "_1", which is permitted to allow a backwards-compatible change with the previous disk format).
Then, only the commands need small updates - "create" now looks up multiple manifests and saves multiple manifests, while "annotate" now annotates all the matching manifests.
- How to verify it
Use docker manifest with multiple docker manifests - e.g.
docker manifest create library/ubuntu library/alpine
- Description for the changelog
Add support for merging manifest lists to docker manifest
- A picture of a cute animal (not mandatory but encouraged)
cc @thaJeztah @crazy-max