Skip to content

Added support for setting OS version in docker manifest annotate.#1461

Closed
SaswatB wants to merge 0 commit intodocker:masterfrom
SaswatB:master
Closed

Added support for setting OS version in docker manifest annotate.#1461
SaswatB wants to merge 0 commit intodocker:masterfrom
SaswatB:master

Conversation

@SaswatB
Copy link
Copy Markdown
Contributor

@SaswatB SaswatB commented Oct 19, 2018

Signed-off-by: Saswat Bhattacharya [email protected]

- What I did
Added support for setting os.version through docker manifest annotate.

- How I did it
Created the flag --os-version that can be passed into docker manifest annotate to update the os version.

- How to verify it

docker manifest create localhost:5000/ubuntu-annotest localhost:5000/ubuntu --insecure
docker manifest annotate localhost:5000/ubuntu-annotest localhost:5000/ubuntu --os-version 1

docker manifest inspect localhost:5000/ubuntu-annotest --insecure
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1150,
         "digest": "sha256:6b9eb699512656fc6ef936ddeb45ab25edcd17ab94901790989f89dbf782344a",
         "platform": {
            "architecture": "amd64",
            "os": "linux",
            "os.version": "1"
         }
      }
   ]
}

- Description for the changelog

Added flag --os-version to docker manifest annotate for updating os version.

- A picture of a cute animal (not mandatory but encouraged)
🐱

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 19, 2018

Codecov Report

Merging #1461 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1461      +/-   ##
=========================================
+ Coverage   54.19%   54.2%   +<.01%     
=========================================
  Files         289     289              
  Lines       19377   19380       +3     
=========================================
+ Hits        10502   10505       +3     
  Misses       8199    8199              
  Partials      676     676

vdemeester
vdemeester approved these changes Oct 22, 2018
View reviewed changes
Copy link
Copy Markdown
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

SGTM 👼

@vdemeester vdemeester requested a review from dmcgowan October 23, 2018 08:00
@vdemeester
Copy link
Copy Markdown
Collaborator

vdemeester commented Oct 23, 2018

/cc @estesp

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 25, 2018

I seem to recall there was a serialized (string) format for platform, which includes all of these parts. Wondering if we need separate flags for all of these, or just a way to pass a platform string (which would then be parsed/splitted into those components)

/cc @tonistiigi @stevvooe

@SaswatB
Copy link
Copy Markdown
Contributor Author

SaswatB commented Nov 10, 2018

It seems we already went with having a separate flag for each field as the os features field is separate. There is a format defined for the whole platform being describe as a single string but os version is not included in that currently.

@GregMialon
Copy link
Copy Markdown

GregMialon commented Jul 22, 2019

@SaswatB, thanks for making the change - what is your expected timeline to have this released?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jul 22, 2019

ping @tonistiigi @dmcgowan PTAL

@SaswatB
Copy link
Copy Markdown
Contributor Author

SaswatB commented Sep 5, 2019

This PR has been open for almost a year, I'll be closing it on October 18th if there's no interest in this being merged.

tonistiigi
tonistiigi approved these changes Sep 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Patch SGTM. Not sure what is the state of manifest command in general though.

@GregMialon
Copy link
Copy Markdown

GregMialon commented Oct 7, 2019

@SaswatB, since the change has been approved, will you be merging it soon? Thanks!

@SaswatB
Copy link
Copy Markdown
Contributor Author

SaswatB commented Oct 7, 2019

@GregMialon I do not have the ability to merge this pr, someone with write access will need to do so.

estesp
estesp approved these changes Oct 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

Seems reasonable given os-features was already present; worth noting that only the Windows runtime uses OS features/version for specific Windows container usage. It doesn't mean someone couldn't use it privately for another use, but on Linux container runtimes (all of them that I'm aware of) these fields are never inspected.

@GregMialon
Copy link
Copy Markdown

GregMialon commented Oct 8, 2019

@thaJeztah , will you be merging this change? Thanks

@SaswatB SaswatB closed this Oct 18, 2019
@GregMialon
Copy link
Copy Markdown

GregMialon commented Nov 5, 2019

@thaJeztah, sorry for the ping again - but what is missing for this to get merged? We were planning to use this for helping us when upgrading the host OS version of Windows - thanks

@cpuguy83
Copy link
Copy Markdown
Collaborator

cpuguy83 commented Jun 12, 2020

Not sure why this got closed. This is very much needed for windows manifests.

@cpuguy83 cpuguy83 reopened this Jun 12, 2020
@cpuguy83 cpuguy83 closed this Jun 12, 2020
@cpuguy83
Copy link
Copy Markdown
Collaborator

cpuguy83 commented Jun 12, 2020

Ok, this is coming from @SaswatB's master branch, github doesn't like me rebasing this (and force closed it). I'll open on another PR.

@cpuguy83 cpuguy83 mentioned this pull request Jun 12, 2020
Merged
@cpuguy83
Copy link
Copy Markdown
Collaborator

cpuguy83 commented Jun 12, 2020

Carried in #2578

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

Reviewers

@vdemeester vdemeester vdemeester approved these changes

@tonistiigi tonistiigi tonistiigi approved these changes

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

@dmcgowan dmcgowan Awaiting requested review from dmcgowan

+1 more reviewer

@estesp estesp estesp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/1-design-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@SaswatB @codecov-io @vdemeester @thaJeztah @GregMialon @cpuguy83 @tonistiigi @estesp @GordonTheTurtle