Considering if we should make this check for the overlap between the given list of platforms and the image that's specified.

Basically thinking;

• docker pull --platform linux/s390x alpine
• I now have alpine on my machine, but only as linux/s390x
• docker save --platform linux/s390x,linux/amd64 alpine (save alpine, but only the s390x and amd64 variants, skip other variants
• ^^ we only have s390x, and "skip" amd64 (which ... isn't present, so nothing to skip)

I guess that depends a bit on how we describe / consider the purpose of --platform in this case;

• Is it a requirement? (export if present, fail if not)
• Is it a filter? (export the given platforms, and skip other ones)

Is it a requirement? (export if present, fail if not)

Yes, I view it as such.

Maybe the better approach is to modify the code you mentioned to error out if more than one platform is passed when using the docker image store? E.g.,

$ docker save --platform linux/s390x,linux/amd64 alpine
<error: --platform only takes a single platform when using the Docker image store.">

Then the user does:

$ docker save --platform linux/amd64 alpine
<error indicating failure because image does not contain specified platform>

$ docker save --platform linux/s390x alpine
<success>

WDYT?

We can now drop the pointer. Before, the pointer was necessary to describe if explicit platform is specified or not.

Makes sense, done.

This (and the other endpoint) could use

Thanks, done.

We should error out if there are more than 1 platforms passed.

Agreed, done.

This matcher is ONLY used for the backwards compatible manifest.json and should still "prefer" the host platform to be used for it.

Yes good catch; I've fixed it. Had to create a new matcher called matchAnyWithPreference which matches any of the given platforms but orders the result so that the host platform comes first (if present in the image manifests).

Nit; trying to avoid future tense;

Make sense, done.

Could use input on this, but changing the signature to return the concrete type may be more convenient;

func matchAnyWithPreference(preferred platforms.MatchComparer, platformList []ocispec.Platform) platformsWithPreferenceMatcher {

At least; locations where it's used should already assert it satisfies the interface, but it can make it more convenient to "jump" to the implementation (i.e., in TestPlatformsWithPreferenceMatcher, clicking matcher.Match in my IDE will directly take me to the implementation, not to the interface.

Agree, done.

Probably not for this PR, but we could even consider merging with the existing matchAllWithPreference, and consider the platform-list to be an optional argument, which would make it something like;

func (c platformsWithPreferenceMatcher) Match(p ocispec.Platform) bool {
	if len(c.platformList) == 0 {
		return true
	}
	return platforms.Any(c.platformList...).Match(p)
}

Mostly looking if we can reduce the number of matcher variations we have; for example, we also have ImageService. matchRequestedOrDefault, which currently accepts a single platform as argument, but (MAYBE?) could take a variadic list or slice, so that we can use the same logic in more places;

Good idea; I added a follow-up commit to the PR, PTAL.

Thanks!

This needs a rebase, and needs to use the new import paths for the api

After rebasing and updating the import paths, it's also needed to run hack/vendor.sh because we now vendor the API module in the main module (don't ask 😆)

I'll open a follow-up to update the docs/api/version-history.md - those docs probably still need to be moved to the API module as well.

Nit (can be updated in a follow-up); platform(s) as name for the structured logs feels a bit awkward; probably either platforms or platform would be better.

No need to change; I initially considered this could be a t.Cleanupin each test; like https://go.dev/play/p/fyK3a91yn5g, but not sure ift.Cleanup` is guaranteed to have finished between each test 🤔 (that way the cleanup - including any failure - was associated with each test).

package main

import "testing"

func TestMultiCleanup(t *testing.T) {
	cleanup := func(t *testing.T) func() {
		return func() {
			t.Helper()
			t.Log("cleaning up", t.Name())
		}
	}

	t.Run("test one", func(t *testing.T) {
		t.Cleanup(cleanup(t))
	})
	t.Run("test two", func(t *testing.T) {
		t.Cleanup(cleanup(t))
		t.Fatal("test two failed")
	})
	t.Run("test three", func(t *testing.T) {
		t.Cleanup(cleanup(t))
	})
}

Alternatively,, we could could setup a fake store as part of a setup() for each test, so that no state is shared between tests which would allow t.Parallel() to work as well.