image-index: define platform.os.version on Windows by AkihiroSuda · Pull Request #651 · opencontainers/image-spec

AkihiroSuda · 2017-04-24T08:26:04Z

Please refer to #632 for previous discussion.

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

wking

I think the alternative to “implementation-defined on all OSes” would be “implementation-defined on windows, and reserved on other OSes”. In the absence of anyone asking for os.version outside of Windows, I'd prefer reserving the field there. But the consistency of “implementation-defined everywhere” is nice too.

wking · 2017-04-24T16:27:31Z

image-index.md

    - **`os.version`** *string*

-        This OPTIONAL property specifies the operating system version, for example `10.0.10586`.
+        This OPTIONAL property specifies the version of the operating system included in the layers specified by the manifest.


The operating system isn't included in the layers. Maybe:

This OPTIONAL property specifies the version of the operating system targeted by the referenced blob.

That also gets us away from using “manifest”, because while image-index.md still talks about manifests as a list of manifests, the non-manifest example in image-layout.md shows that manifests is no longer restricted to just manifests. I'd thought the wording around manifests was going to be generalized, but that doesn't seem to have happened yet. I think changing the manifests wording is out of scope for this PR, but I'd rather not dig the current hole any deeper ;).

wking · 2017-04-24T16:28:13Z

image-index.md


-        This OPTIONAL property specifies the operating system version, for example `10.0.10586`.
+        This OPTIONAL property specifies the version of the operating system included in the layers specified by the manifest.
+        Implementations MAY refuse to use manifests of which `os.version` is not known to work with the host OS version.


“of which” → “if”? Or maybe “where”?

wking · 2017-04-24T16:29:00Z

image-index.md

-        This OPTIONAL property specifies the operating system version, for example `10.0.10586`.
+        This OPTIONAL property specifies the version of the operating system included in the layers specified by the manifest.
+        Implementations MAY refuse to use manifests of which `os.version` is not known to work with the host OS version.
+        When OS is Windows, image indexes SHOULD use, and implementations SHOULD understand quad-notation values of [`System.Version` properties](system-version), `Major.Minor.Build.Revision`. e.g. `10.0.14393.1066`.


“OS is Windows” → “os is windows”, and similarly in the following line. And I think we want to be implementation-defined for all OSes, with the Windows matching so fuzzy. @jstarks seemed to be ok with that approach.

wking · 2017-04-25T16:52:02Z

image-index.md

-        This OPTIONAL property specifies the operating system version, for example `10.0.10586`.
+        This OPTIONAL property specifies the version of the operating system targeted by the referenced blob.
+        Implementations MAY refuse to use manifests where `os.version` is not known to work with the host OS version.
+        When `os` is `windows`, image indexes SHOULD use, and implementations SHOULD understand quad-notation values of `Major.Minor.Build.Revision`. e.g. `10.0.14393.1066`.


I still think we want this to be implementation-defined on Windows to avoid having to specify the fuzzy matching needed for it to be useful. And @jstarks seemed to be ok with that approach.

wking · 2017-04-26T16:16:21Z

image-index.md

+        This OPTIONAL property specifies the version of the operating system targeted by the referenced blob.
+        Implementations MAY refuse to use manifests where `os.version` is not known to work with the host OS version.
+        The values are implementation-defined when `os` is `windows`. e.g. `10.0.14393.1066`.
+        The values for other `os`-es SHOULD be submitted to this specification for standardization.


20dcb7a → de499b4 dropped implementation-defined for non-Windows os. I think the full section should read:

This OPTIONAL property specifies the version of the operating system targeted by the referenced blob. Implementations MAY refuse to use manifests where `os.version` is not known to work with the host OS version. Valid values are implementation-defined.

Once we make the values implementation-defined, there's no clawing it back into the spec without risking breaking changes, so I don't think we should bother asking for submission for standardization.

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

vbatts · 2017-05-03T19:03:18Z

@RobDolinMS can you get some Windows eyes to do a review of this

darstahl · 2017-05-12T20:37:37Z

Not a maintainer, but LGTM

vbatts · 2017-05-15T14:44:54Z

Thanks @darrenstahlmsft
LGTM
(to echo his approval)

vbatts · 2017-05-19T18:01:10Z

review please

stevvooe · 2017-05-23T17:58:35Z

LGTM

AkihiroSuda mentioned this pull request Apr 24, 2017

image-index: mark os.version, os.features and variant as "reserved" #632

Closed

wking reviewed Apr 24, 2017

View reviewed changes

AkihiroSuda force-pushed the platform-os-version branch from 9ac7ffc to 20dcb7a Compare April 25, 2017 06:26

wking reviewed Apr 25, 2017

View reviewed changes

AkihiroSuda force-pushed the platform-os-version branch 2 times, most recently from cc6c36f to de499b4 Compare April 26, 2017 02:19

wking reviewed Apr 26, 2017

View reviewed changes

image-index: define platform.os.version on Windows

bc58ab2

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

AkihiroSuda force-pushed the platform-os-version branch from de499b4 to bc58ab2 Compare April 27, 2017 06:43

wking approved these changes Apr 27, 2017

View reviewed changes

vbatts mentioned this pull request May 3, 2017

improve definition of platform.features #622

Closed

stevvooe added this to the v1.0.0 milestone May 12, 2017

stevvooe merged commit 4f04bc4 into opencontainers:master May 23, 2017

wking mentioned this pull request May 23, 2017

Expand platform to be more sophisticated opencontainers/runtime-spec#830

Closed

Conversation

AkihiroSuda commented Apr 24, 2017

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented May 3, 2017

Uh oh!

darstahl commented May 12, 2017

Uh oh!

vbatts commented May 15, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbatts commented May 19, 2017

Uh oh!

stevvooe commented May 23, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vbatts commented May 15, 2017 •

edited by caniszczyk

Loading

stevvooe commented May 23, 2017 •

edited by caniszczyk

Loading