Embed Platform in Image

[thaJeztah]

• relates to the discussion on Fix several conversions of "ocispec.Image" to "ocispec.Platform" containerd/containerd#7376 (comment)

This embeds the Platform struct in the image struct, so that these fields don't have to be defined multiple times, and to allow projects that have tooling for validating/normalizing/handling Platform data can apply these on an image-config, without first constructing a Platform from these fields.

[vrothberg]

LGTM

[sudo-bmitch]

This will be a breaking change for anyone that creates a struct like:

	conf := v1.Image{
		Created: &now,
		OS:           "linux",
		Architecture: "amd64",
		RootFS: v1.RootFS{
			Type:    "layers",
			DiffIDs: []digest.Digest{},
		},
	}

I'm okay with that since I can't think of a non-breaking way to change this, and it cleans up so many other things. The fix is also trivial for anyone that encounters it:

	conf := v1.Image{
		Created: &now,
		Platform: v1.Platform{
			OS:           "linux",
			Architecture: "amd64",
		},
		RootFS: v1.RootFS{
			Type:    "layers",
			DiffIDs: []digest.Digest{},
		},
	}

[thaJeztah]

Thank you both for looking! I opened this PR in between zoom calls, and started to write up a bit about the incompatible change (from a Golang perspective), but time got the best of me 😅

So, while this PR is effectively the equivalent of the "non-embedded" variant (with of course the advantages that the types are now properly "glued" together to prevent them accidentally diverging), it is indeed an incompatible change when using a struct-literal.

I expect that most (if not all) immediate consumers of the image-spec code to be fairly "seasoned" Go project, and fixing such cases, while annoying, should not take more than a few minutes. I did a quick test PR in containerd after I opened this PR, and in that repository only one instance had to be changed (in test-code, not production code); containerd/containerd#7382

That said, it is possible that other projects could run into a situation where indirect consumers of this code can run into issues (i.e. if two dependencies (say, <module X> and <module Y> both use a struct literal, and only one of them being upgraded for the new struct).

The "correct" thing to do (as go modules follow SemVer) would be to update the major version, which would effectively mean; a new module, named github.com/opencontainers/image-spec/v2/, which would allow <module X> and <module Y> to use different major versions of the image-spec go module. Of course, "correct" here would also be "disruptive"; some of that outlined in a ticket I created some time ago in the runtime-spec; opencontainers/runtime-spec#1069 (comment)

• Currently, the go module follows the same versioning as the specification itself, which is less confusing (v1.0.0 of the go module is also v1.0.0 of the specification). But effectively both "golang" and the specification are competing for ownership of the SemVer tag; a non-breaking change in the spec may be a breaking change for the Go module, and vice-versa.
• Decoupling their versions would resolve that, but definitely more confusing ("module" v2.0.x implements v1.0.x of the spec), so not ideal.
• Go module's support for multiple major versions can be disruptive (as updating to a new major version requires a new module name, so all import paths must be updated); projects depending on a module will also not automatically update to new major versions, which can lead to projects sticking to old major versions (they may not be aware there's a new major version).
• And while major versions may resolve the immediate issue for the "module X and module Y" case, it's possible that those modules need to interact, and things may still break and/or glue code may need to be added for conversion. (Technically those modules would now also be forced to update to a new major version).

So, what's best? Perhaps there isn't that much breakage and we use GoVer ("like SemVer, but never reaches v2" 😂). It may be good to do some test pull requests in projects to see if this change would break things (similar to containerd/containerd#7382); I'm planning to do some of those in at least Moby, BuildKit, distribution to see if there's breakage.

Opened;

• [do not merge] test image-spec with embedded Platform containerd/containerd#7382
• [do not merge] test image-spec with embedded Platform moby/buildkit#3102
• [do not merge] test image-spec with embedded Platform moby/moby#44124
• [do not merge] test image-spec with embedded Platform distribution/distribution#3732

[sudo-bmitch]

My hope is this is a narrow enough use case that we can get away with a minor version bump. I tested on regclient, and only ran into a single scenario. For anyone accessing elements of the struct after it's created, and marshalling/unmarshalling from json, this will be completely transparent. Hopefully that's almost all the current use cases.

[thaJeztah]

👍 I'm 100% with you; I just opened some PRs (linked them from my previous comment); had to make some changes, but it's looking good.

[sajayantony]

@tianon you did a ❤️ response and I'm reading this as general goodness. Do you want to raise this on the runtime side to see if there are any concerns merging this?

[tianon]

If by "runtime side" you mean the runtime-spec, then there can't be because it doesn't deal with platform objects at all. 😇

If you mean actual container runtimes, I think this change itself was inspired by my PR at containerd/containerd#7376 where I even suggested that this was probably a correct fix but didn't have the spoons to make the PR myself and Seb apparently does. 😅

I'm definitely impressed (not surprised, but still impressed ❤️) at the level to which he's taken it, making test PRs in several downstream consumers to see where this might break folks and how cumbersome the fix is (which is to say, not much).

I find myself wishing Go had a cleaner mechanism for these types of inter-struct relationships ("image 'is' a platform"), perhaps more similar to the interface concept, but it's always a tradeoff and I do not envy people who try to design programming languages. 😂

I guess that's the really long way of saying (TLDR) "I'm not an image-spec maintainer, but I think this is a good and correct change" 😅 ❤️

[sudo-bmitch]

We reviewed this one in today's meeting and agreed to move forward since the breaking change is only to a handful of Go use cases and not to the spec itself. Thanks for driving this @thaJeztah!

[thaJeztah]

Thanks!

I should try to join the OCI calls, but never make it 😞 😅