c8d/pull: Same error message for non-matching platform#48414
c8d/pull: Same error message for non-matching platform#48414vvoland merged 1 commit intomoby:masterfrom
Conversation
Use the same error message as the graphdrivers image store backend. It's more informative as it also includes the requested platform and won't break clients checking doing error check with string-matching. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
| // Transform "no match for platform in manifest" error returned by containerd into | ||
| // the same message as the graphdrivers backend. | ||
| // The one returned by containerd doesn't contain the platform and is much less informative. | ||
| if strings.Contains(err.Error(), "platform") { |
There was a problem hiding this comment.
😢 on the string-matching. Guess we have no real other option here 😞
Wondering if there would be options to do so through richer errors from the containerd code 🤔 (or perhaps the containerd code to return a better error-message)
There was a problem hiding this comment.
Went looking if that's something the matcher could return, but that's using a simple bool as interface, so no options there 😢
moby/vendor/github.com/containerd/platforms/platforms.go
Lines 133 to 136 in 5ea96ca
| // the same message as the graphdrivers backend. | ||
| // The one returned by containerd doesn't contain the platform and is much less informative. | ||
| if strings.Contains(err.Error(), "platform") { | ||
| platformStr := platforms.DefaultString() |
There was a problem hiding this comment.
Just a note; this also includes the os-version on Windows. I don't think we show that in other places, but perhaps it's good to show, as we would be matching using that (I think?)
There was a problem hiding this comment.
Yes, IMO it makes sense to show it. Although it will only really be matched on the windows host due to different containerd platform matcher implementation based on the target platform the code was built for.
| if platform != nil { | ||
| platformStr = platforms.Format(*platform) | ||
| } | ||
| return errdefs.NotFound(fmt.Errorf("no matching manifest for %s in the manifest list entries: %w", platformStr, err)) |
There was a problem hiding this comment.
FWIW, I'd be somewhat ok with changing this to no match for platform <platform> as it's probably more user-friendly, but yeah, not sure if anyone is expecting this error message 🤔
Use the same error message as the graphdrivers image store backend. It's more informative as it also includes the requested platform and won't break clients checking doing error check with string-matching.
- What I did
- How I did it
- How to verify it
CI
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)