Add osversion and osvariant to platform specification by jstarks · Pull Request #1534 · distribution/distribution

jstarks · 2016-03-16T02:31:26Z

These are needed to differentiate incompatible Windows versions. In the short run, Windows will need os, osvariant, and osversion matching. Eventually we should be able to avoid matching on osvariant.

cc/@aaronlehmann

friism · 2016-03-16T02:49:48Z

cc @dmp42

dmp42 · 2016-03-16T06:24:38Z

@docker/core-distribution-maintainers please take a look at this

dmp42 · 2016-03-16T06:26:04Z

@jstarks be sure to validate the design of this with @aaronlehmann @dmcgowan and @RichardScothern

jstarks · 2016-03-16T16:17:55Z

Since last night I thought about this some more, and I think osvariant is a mistake -- it's too coarse. What would be best would be either:

osfeatures: An array of OS features that the image requires to run. For Windows, the first feature we would need would be the presence or absence of the graphics stack, which is what keeps Server Core images from working on Nano Server.
features: Use the existing features, but namespace it, e.g. we could have a feature "windows:graphics" or "os:graphics" or something like that.

Thoughts?

aaronlehmann · 2016-03-16T17:02:25Z


+	// OSVersion is an optional field specifying the operating system
+	// version, for example `10.0.10586`.
+	OSVersion string `json:"osversion"`


Let's mark this one omitempty too. There will be a lot of cases where OS version requirements are unspecified, such as most Linux images.

Oops, yes that was my intention.

aaronlehmann · 2016-03-16T17:04:53Z

I agree that having a set of OS features is better than having a single string. I don't have much of a preference either way between using the existing features array or adding a new one for OS-specific feature requirements. Does anyone else have thoughts on this?

jstarks · 2016-03-16T17:32:14Z

I've updated the PR with a proposal that expands the meaning of features (and fixed the missing omitempty, which also fixes CI).

codecov-io · 2016-03-16T17:44:01Z

Current coverage is `51.22%`

Merging #1534 into master will decrease coverage by -6.71% as of 5b0a484

@@            master   #1534   diff @@
======================================
  Files          122     122       
  Stmts        10612   10605     -7
  Branches       838     744    -94
  Methods          0       0       
======================================
- Hit           6148    5432   -716
+ Partial        838     744    -94
- Missed        3626    4429   +803

Review entire Coverage Diff as of 5b0a484

Uncovered Suggestions

+0.30% via .../driver/s3-aws/s3.go#730...761
+0.28% via .../driver/s3-aws/s3.go#266...295
+0.21% via .../driver/s3-aws/s3.go#882...904
See 7 more...

Powered by Codecov. Updated on successful CI builds.

stevvooe · 2016-03-16T23:52:30Z

            The os field specifies the operating system, for example
            `linux` or `windows`.

+        - **`osversion`** *string*


"os.version" 🏠 🚲

I'm open to that, but it's a little strange since . is the field separator in most languages. This means that if you were manipulating this in JavaScript, for example, you couldn't write p.os.version, you'd have to write p["os.version"]. But AFAIK, if it's osversion, you can just write p.osversion.

Agreed.

If we can come up with a way to namespace the properties as under "os", that would be sufficient. I don't have a suggestion off the top of my head.

Does os_version fit the bill?

Ultimately this is up to you all. I'm happy to choose os.version if that's what you'd like.

jstarks · 2016-03-17T21:07:14Z

I've updated the branch with os.version and os.features. I'm not sure if the bikeshedding is complete or not ;)

stevvooe · 2016-03-17T23:31:36Z

@jstarks I didn't see your comment earlier. I found you point about p.osversion value, although, I prefer this.

stevvooe · 2016-03-17T23:32:52Z

+        - **`os.features`** *array*
+
+            The optional os.features field specifies an array of strings,
+            each listing a required CPU feature (for example on Windows


I'm a little confused here. Are these CPU features or features provided by the os? Maybe some documentation on this would help to clarify.

Oops, sorry, that should say OS feature.

Whew! Thought I had misunderstood and had you going around making new fields.

Awesome. LGTM after correction!

stevvooe · 2016-03-17T23:33:40Z

LGTM

There are some clarifications of the role of "os.features" but the naming looks fine.

These changes are needed to differentiate Windows images. Signed-off-by: John Starks <[email protected]>

aaronlehmann · 2016-03-21T23:17:13Z

LGTM

Add osversion and osvariant to platform specification

aslanpour · 2022-07-23T00:52:29Z

Hi All,

I am still not sure where the values for os.features come from in a Linux OS. Is that equivalent to the values for CPU features in "cat /proc/cpuinfo"?

milosgajdos · 2022-07-24T08:43:22Z

Hi All,

I am still not sure where the values for os.features come from in a Linux OS. Is that equivalent to the values for CPU features in "cat /proc/cpuinfo"?

You might wanna ask in https://github.com/moby/moby/

GordonTheTurtle added status/0-triage group/windows labels Mar 16, 2016

aaronlehmann reviewed Mar 16, 2016
View reviewed changes

jstarks force-pushed the windows_platform branch from 81860e8 to dd0959e Compare March 16, 2016 17:31

stevvooe reviewed Mar 16, 2016
View reviewed changes

jstarks force-pushed the windows_platform branch from dd0959e to 24742e4 Compare March 17, 2016 21:06

stevvooe reviewed Mar 17, 2016
View reviewed changes

Add os.version and os.features to platform spec

5b0a484

These changes are needed to differentiate Windows images. Signed-off-by: John Starks <[email protected]>

jstarks force-pushed the windows_platform branch from 24742e4 to 5b0a484 Compare March 17, 2016 23:54

aaronlehmann pushed a commit that referenced this pull request Mar 21, 2016

Merge pull request #1534 from jstarks/windows_platform

c8d8e7e

Add osversion and osvariant to platform specification

aaronlehmann merged commit c8d8e7e into distribution:master Mar 21, 2016

aaronlehmann mentioned this pull request Mar 30, 2016

Add os_version and os_features to Image moby/moby#21272

Merged

Conversation

jstarks commented Mar 16, 2016

Uh oh!

friism commented Mar 16, 2016

Uh oh!

dmp42 commented Mar 16, 2016

Uh oh!

dmp42 commented Mar 16, 2016

Uh oh!

jstarks commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 16, 2016

Uh oh!

jstarks commented Mar 16, 2016

Uh oh!

codecov-io commented Mar 16, 2016

Current coverage is 51.22%

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!

jstarks commented Mar 17, 2016

Uh oh!

stevvooe commented Mar 17, 2016

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!

stevvooe commented Mar 17, 2016

Uh oh!

aaronlehmann commented Mar 21, 2016

Uh oh!

aslanpour commented Jul 23, 2022

Uh oh!

milosgajdos commented Jul 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Current coverage is `51.22%`