Add manifest merging support #3994
Open
+169
−145
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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>
jedevc
force-pushed
the
manifest-merging
branch
from
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 |
jedevc
commented
Feb 8, 2023
|
|
||
| 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.
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?
thaJeztah
reviewed
Feb 14, 2023
| } | ||
|
|
||
| // NotFound interface | ||
| func (n *notFoundError) NotFound() {} |
There was a problem hiding this comment.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
Introduced support for to merge manifest lists and OCI indexes to the
docker manifestcommand.- 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