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

image-index: define platform.os.features on Windows #652

Merged
merged 1 commit into from
Jun 28, 2017
Merged

image-index: define platform.os.features on Windows #652

merged 1 commit into from
Jun 28, 2017

Conversation

AkihiroSuda
Copy link
Member

Please refer to #632 for previous discussion.

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

@AkihiroSuda AkihiroSuda mentioned this pull request Apr 24, 2017
Closed
wking
wking reviewed Apr 24, 2017
View reviewed changes
image-index.md Outdated
This OPTIONAL property specifies an array of strings, each specifying a mandatory OS feature.
For Windows, image indexes SHOULD use, and implementations SHOULD understand the following values:
- `win32k`: image requires `win32k.sys` on the host (Note: `win32k.sys` is missing on Nano Server)
Convention for other features SHOULD be submitted to this specification for standardization.
Copy link
Contributor

@wking wking Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I liked your #650 wording for this better. How about:

When os is not windows, values are implementation-defined and SHOULD be submitted to this specification for standardization.

Alternatively, I'm fine reserving this for non-Windows OSes.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@AkihiroSuda
image-index: define platform.os.features on Windows
922ccc1 
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda AkihiroSuda force-pushed the platform-os-features branch from 5f52aba to 922ccc1 Compare April 25, 2017 06:23
@wking wking mentioned this pull request Apr 27, 2017
Closed
2 tasks
@vbatts
Copy link
Member

vbatts commented May 1, 2017

@RobDolinMS can you get someone to review this, please?

@vbatts vbatts mentioned this pull request May 3, 2017
Closed
@stevvooe stevvooe added this to the v1.0.0 milestone May 12, 2017
@wking wking mentioned this pull request May 16, 2017
Closed
@AkihiroSuda
Copy link
Member Author

@RobDolinMS PTAL?

@RobDolinMS
Copy link
Collaborator

@darrenstahlmsft would you please give this a look?

@RobDolinMS
Copy link
Collaborator

Tagging @jstarks to take a look.

@vbatts
Copy link
Member

vbatts commented May 25, 2017

bump

@RobDolinMS
Copy link
Collaborator

RobDolinMS commented May 31, 2017 •
edited
Loading

@jstarks has been out-of-office but he should be back soon and has been asked to review.

(@vbatts has said he will give this his LGTM if John Starks is a thumbs-up)

@zhouhao3
Copy link

Waiting for results.

@vbatts
Copy link
Member

vbatts commented Jun 21, 2017

we're 30+ days on getting a review for this. I'm in favor of bumping this to post 1.0 so it is not a blocker

@philips
Copy link
Contributor

philips commented Jun 23, 2017

100%, lets move to post 1.0

@stevvooe
Copy link
Contributor

I move to open another PR to simply reserve these fields.

@AkihiroSuda Do you mind leaving this PR open and we'll reserve these fields for a future version?

@AkihiroSuda
Copy link
Member Author

no problem

@vbatts
Copy link
Member

vbatts commented Jun 26, 2017

@stevvooe so, post-v1.0.0? or are you opening a PR for the reserve language?

@jstarks
Copy link
Contributor

jstarks commented Jun 28, 2017

Maybe this is a day late and a dollar short, but LGTM.

@vbatts
Copy link
Member

vbatts commented Jun 28, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

1 similar comment
@stevvooe
Copy link
Contributor

stevvooe commented Jun 28, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

@stevvooe stevvooe merged commit 28462ef into opencontainers:master Jun 28, 2017
@vbatts vbatts mentioned this pull request Jul 5, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wking wking wking left review comments

Assignees
No one assigned
Labels
priority/Pmaybe
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@AkihiroSuda @vbatts @RobDolinMS @zhouhao3 @philips @stevvooe @jstarks @wking