Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Embed platform fields in Image spec #809

Merged
merged 2 commits into from Jul 29, 2021
Merged

Embed platform fields in Image spec #809

merged 2 commits into from Jul 29, 2021

Conversation

cpuguy83
Copy link
Contributor

Carries #777 + adds other platform fields
Closes #777

@cpuguy83 cpuguy83 mentioned this pull request Sep 30, 2020
Closed
@tianon
Copy link
Member

tianon commented Sep 30, 2020

(not a maintainer) I'm +1 on this, but I think it's probably worth at least mentioning pros/cons of including all the platform fields vs deprecating existing fields and adding a full proper platform object instead. 😇

@cpuguy83
Copy link
Contributor Author

Major con I can see with that is builders will need to set both the old fields and the new platform field to maintain backwards compatibility.

Also requires allocating another object, but 🤷

@caniszczyk
Copy link
Contributor

@cyphar any comments here?

ping @opencontainers/image-spec-maintainers

It seems this would warrant a bump in a minor or micro release, nothing breaks here (optional field)

cyphar
cyphar previously requested changes Oct 1, 2020
View reviewed changes
config.md Outdated Show resolved Hide resolved
conversion.md Outdated Show resolved Hide resolved
conversion.md Outdated Show resolved Hide resolved
@cyphar
Copy link
Member

cyphar commented Oct 1, 2020

I think a platform object would be a nicer solution, but 🤷 from my side too.

@cpuguy83
Copy link
Contributor Author

cpuguy83 commented Oct 1, 2020

Updated.

@cpuguy83
Copy link
Contributor Author

Ping

@cpuguy83 cpuguy83 requested a review from cyphar October 12, 2020 18:17
@caniszczyk
Copy link
Contributor

ping @opencontainers/image-spec-maintainers

vrothberg
vrothberg reviewed Mar 10, 2021
View reviewed changes
Copy link
Contributor

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, @cpuguy83!

I would love to help things push forward if needed. @opencontainers/image-spec-maintainers, are there any open issues that prevent the changes from getting in?

I have an immediate need to know the variant of an image. Once an image was pulled, we lose that information which is currently causing me headaches.

@jonjohnsonjr
Copy link
Contributor

You could poke some folks in the weekly meeting (today) if you want to get some activity.

@vrothberg vrothberg mentioned this pull request Mar 10, 2021
Closed
@vrothberg
Copy link
Contributor

vrothberg commented Mar 11, 2021 •
edited

You could poke some folks in the weekly meeting (today) if you want to get some activity.

Thanks. I wanted to join for a long time but the time is tough (11PM in my time zone). Maybe, I'll make it once to help push this issue forward :)

vbatts
vbatts reviewed Jul 9, 2021
View reviewed changes
conversion.md Show resolved Hide resolved
vbatts
vbatts previously approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lines up with what's already being done (apart from my one comment), and it matches with the os and variant fields in the image-index descriptors.

jonjohnsonjr
jonjohnsonjr reviewed Jul 9, 2021
View reviewed changes
schema/validator.go Outdated Show resolved Hide resolved
tianon
tianon reviewed Jul 9, 2021
View reviewed changes
schema/validator.go Outdated Show resolved Hide resolved
@cpuguy83
Add CPU variant to image config
0e20f8a 
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>
@cpuguy83
Copy link
Contributor Author 

# cd .; git clone https://github.com/opencontainerd/image-spec /home/runner/work/image-spec/image-spec/go/src/github.com/opencontainerd/image-spec
Cloning into '/home/runner/work/image-spec/image-spec/go/src/github.com/opencontainerd/image-spec'...
fatal: could not read Username for 'https://github.com': terminal prompts disabled
package github.com/opencontainerd/image-spec/specs-go/v1: exit status 128

@vbatts
Copy link
Member

vbatts commented Jul 10, 2021

@cpuguy83 hrm. Is that a go.mod issue?

@tianon
Copy link
Member

tianon commented Jul 10, 2021 via email

@vbatts
Copy link
Member

vbatts commented Jul 12, 2021

/me kicked the jobs

jonjohnsonjr
jonjohnsonjr reviewed Jul 12, 2021
View reviewed changes
schema/validator.go Outdated Show resolved Hide resolved
@cpuguy83
Embedded other platform fields in image spec
170393e 
This makes sure that an index can be generated from an image spec.

`OSVersion` in particular is fairly important for Windows since Windows
can only run containers for the OS version it was built for, and
currently the only way to reliably get this is from the index.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Copy link
Contributor Author

All green now.

stevvooe
stevvooe approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83
Copy link
Contributor Author

@cyphar Does this LGTY? I made the adjustments you'd requested before. Merging is currently blocked due to prior review.

Thanks!

@vbatts vbatts dismissed cyphar’s stale review July 29, 2021 14:25

prior review is outdated

vbatts
vbatts approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😸

@vbatts vbatts merged commit fe0a249 into opencontainers:main Jul 29, 2021
@cpuguy83 cpuguy83 deleted the image-platform branch July 29, 2021 14:26
@estesp estesp mentioned this pull request Feb 18, 2022
Closed
@sudo-bmitch sudo-bmitch mentioned this pull request Sep 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tianon tianon tianon left review comments

@vrothberg vrothberg vrothberg left review comments

@vbatts vbatts vbatts approved these changes

@stevvooe stevvooe stevvooe approved these changes

@jonjohnsonjr jonjohnsonjr jonjohnsonjr approved these changes

@cyphar cyphar Awaiting requested review from cyphar cyphar is a code owner

@brendandburns brendandburns Awaiting requested review from brendandburns

@jonboulle jonboulle Awaiting requested review from jonboulle jonboulle is a code owner

@jstarks jstarks Awaiting requested review from jstarks

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@cpuguy83 @tianon @caniszczyk @cyphar @jonjohnsonjr @vrothberg @vbatts @stevvooe