Add the option to choose an image by the variant field in a manifest #854
Conversation
|
@vrothberg PTAL, I think this (and #820) are the major things worth waiting for before a release. |
mtrmac
changed the title
vrothberg
left a comment
vrothberg
left a comment
There was a problem hiding this comment.
Just a couple of nits.
I wished we wouldn't have to hard-code all the different variants; it feels like whack a mole on the long run. But I don't have an alternative at hand.
copy/copy.go
Outdated
|
|
||
| "github.com/containers/image/v5/docker/reference" | ||
| "github.com/containers/image/v5/image" | ||
| platform "github.com/containers/image/v5/internal/pkg/platform" |
There was a problem hiding this comment.
This seems redundant (and smells like VSCode?).
copy/copy.go
Outdated
| wantedArch = sys.ArchitectureChoice | ||
| // runtimeMatchWarning returns an error message warning about the mismatch | ||
| // between an image config and set of platforms by a runtime, or "" if the two match. | ||
| func runtimeMatchWarning(cfg *imgspecv1.Image, platforms []imgspecv1.Platform) string { |
There was a problem hiding this comment.
Nit: Using an error (instead of a string) as return value seems more idiomatic to me. Can we change it?
copy/copy.go
Outdated
| matches = false | ||
| } | ||
| if matches { | ||
| break |
copy/copy.go
Outdated
| for _, o := range platforms { | ||
| options.append(fmt.Sprintf("%s+%s", o.OS, o.Architecture)) | ||
| } | ||
| return fmt.Sprintf("Image operating system mismatch: image uses OS %q+architecture %q, expecting one of %q", |
There was a problem hiding this comment.
I prefer to only keep the default case here. I don't like the precedence of OS over the arch as both could very well mismatch.
There was a problem hiding this comment.
It would simply this function quite a bit as well.
There was a problem hiding this comment.
Sure, the default can be good enough.
In that case I’m tempted to inline the warning generator back and drop the tests. WDYT?
|
@mtrmac, sorry for not mentioning earlier. Could you squash the commits into one? |
|
Updated and squashed now. |
|
LGTM |
…tibility checks Autodetection of the current variant is largely based on https://github.com/moby/moby/blob/bc846d2e8fe5538220e0c31e9d0e8446f6fbc022/distribution/cpuinfo_unix.go https://github.com/containerd/containerd/blob/726dcaea50883e51b2ec6db13caff0e7936b711d/platforms/cpuinfo.go Signed-off-by: Jirka Chadima <chadima.jiri@gmail.com> Signed-off-by: Miloslav Trmač <mitr@redhat.com>
4 participants
This carries over #786 , updating it on top of the later changes to warn instead of reporting an error.
Then, the error messages were updated (now that they don’t abort) to make more sense with multiple platform options. See the added test for an example.
See individual commit messages for details.