Skip to content

Add CPU variant to image config#777

Closed
pricec wants to merge 1 commit into
opencontainers:mainfrom
pricec:image-config-variant
Closed

Add CPU variant to image config#777
pricec wants to merge 1 commit into
opencontainers:mainfrom
pricec:image-config-variant

Conversation

@pricec
Copy link
Copy Markdown

@pricec pricec commented May 6, 2019

This commit adds the CPU variant to the image config type. It also
refactors the image index specification to isolate the platform
variant specifications, allowing a reference from the config spec.
The go specs are updated to include the new field in the v1.Image
struct, and tests are updated to include the new field.

Signed-off-by: Chris Price chris.price@docker.com

This commit adds the CPU variant to the image config type. It also
refactors the image index specification to isolate the platform
variant specifications, allowing a reference from the config spec.
The go specs are updated to include the new field in the v1.Image
struct, and tests are updated to include the new field.

Signed-off-by: Chris Price <chris.price@docker.com>
@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented May 6, 2019

This is also upstreaming a change in buildkit and soon to be in moby. https://github.com/moby/buildkit/blob/9a18f8040332acebbb8a9a21772560ab99d7fdaa/frontend/dockerfile/dockerfile2llb/image.go#L55

Note this change is useful for builders which create indexes from a list of manifests. The manifest links to the config, however today the config does not contain all the information needed to correctly construct an index containing 32-bit ARM images. A larger change such as adding the entire platform would complicate client compatibility, further hindering the ARM use cases. With this change clients need only to update the Image structure and use the variant if copying to a platform structure or no change at all if parsing into a platform structure, however, adding a new platform field would require clients to update their Image structure and add logic to clean up the structure EVERY time the structure is parsed just to support this ARM use case.

@vbatts
Copy link
Copy Markdown
Member

vbatts commented Dec 17, 2019

LGTM

Approved with PullApprove

@tianon
Copy link
Copy Markdown
Member

tianon commented Aug 12, 2020

This came up in today's OCI call (cc @ibuildthecloud), and I think this only solves part of the issue, right? We're still missing os.version and friends needed for Windows, so wouldn't it make more sense to add the full platform object to the image config? (at the very least, all the fields it supports would be useful)

@Toasterson
Copy link
Copy Markdown

Adding the full platform object to the Image config has the added bonus of keeping things in sync later down the line.

@jonjohnsonjr
Copy link
Copy Markdown
Contributor

Adding the full platform object to the Image config has the added bonus of keeping things in sync later down the line.

I'd be in favor of embedding the whole v1.Platform into v1.Image to achieve this.

@cpuguy83
Copy link
Copy Markdown
Contributor

Agree, let's bring in the whole platform object.
Does this need to be carried?

@cpuguy83
Copy link
Copy Markdown
Contributor

Carried these changes in #809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants