Build succeeded.

• containerd-build-arm64 : SUCCESS in 11m 27s (non-voting)

Codecov Report

Merging #3609 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3609   +/-   ##
=======================================
  Coverage   42.38%   42.38%           
=======================================
  Files         126      126           
  Lines       13900    13900           
=======================================
  Hits         5891     5891           
  Misses       7123     7123           
  Partials      886      886

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95301fe...9996b75. Read the comment docs.

Codecov Report

Merging #3609 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3609   +/-   ##
=======================================
  Coverage   42.38%   42.38%           
=======================================
  Files         126      126           
  Lines       13901    13901           
=======================================
  Hits         5892     5892           
  Misses       7123     7123           
  Partials      886      886

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b039c39...555cb31. Read the comment docs.

The current interface is limiting, however, passing the platform in does not seem like the ideal interface either. Could we have the default platform as configurable within the client. I am assuming that in the case of LCOW support we will want a platform matcher which can match both LCOW and WCOW and handle the ordering between those. How are these functions expected to be invoked?

@dmcgowan The platform matcher for WCOW (process isolation) is different from WCOW (hyper-V), because the OS version has to be matched. https://github.com/Random-Liu/cri-containerd/blob/windows-support/pkg/containerd/platforms/default_windows.go

Having it in upstream containerd means that we need different matchers for WCOW (process isolation), WCOW (hyperV) and LCOW. Not sure whether we want to do that.

Examples of how these functions will be invoked https://github.com/Random-Liu/cri-containerd/blob/windows-support/pkg/server/image_pull.go#L224

The invocation you linked seems like it could also work with just setting the default platform matcher in the client, rather than using a global default. This would make the client more configurable for those cases where you want the default PlatformMatcher to either be something custom like you linked to, a matcher which is capable of being configured to match both LCOW and WCOW (with an variation of using os version).

@dmcgowan Setting a default in the client works much better for me. I thought about that, but not sure whether people will be OK with that slightly bigger change. :)

If we all think that's better, let's do that then! :D

@dmcgowan Done

I think I also need to handle platforms.DefaultString(). Will do soon.

Build succeeded.

• containerd-build-arm64 : SUCCESS in 6m 46s (non-voting)

I think I also need to handle platforms.DefaultString(). Will do soon.

NVM. That is used for runtime (runtime spec) selection, which doesn't seem related to the image platform matcher.

@katiewasnothere - FYI

@dmcgowan - Anything else you want here. LGTM