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.

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

Use commit-sha instead of tag for containerd #35770

Merged
merged 1 commit into from Dec 13, 2017

Conversation

Projects
None yet
6 participants
@thaJeztah
Member

thaJeztah commented Dec 12, 2017

The docker info command compares the installed version
of containerd using a Git-sha. We currently use a tag for
this, but that tag is not returned by the version-API of
containerd, resulting in the docker info output to show:

containerd version: 89623f28b87a6004d4b785663257362d1658a729 (expected: v1.0.0)

This patch changes the v1.0.0 tag to the commit that
corresponds with the tag, so that the docker info output
does not show the expected: string.

This should be considered a temporary workaround; the check
for the exact version of containerd that's installed was needed
when we still used the 0.2.x branch, because it did not have
stable releases yet.

With containerd reaching 1.0, and using SemVer, we can likely
do a comparison for "Major" version, or make this a "packaging"
issue, and remove the check entirely (we can still print the
version that's installed if we think it's usefule).

I can add a test for this

- How to verify it

run docker info and check that the expected: string is not present

- Description for the changelog

* Fix `docker info` showing "expected: v1.0.0" for containerd [moby/moby#35770](https://github.com/moby/moby/pull/35770)

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

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Dec 12, 2017

Member

IMO the tag is superior, and this would be a good case for fixing the version comparison code, but I could be convinced. 😇

Member

tianon commented Dec 12, 2017

IMO the tag is superior, and this would be a good case for fixing the version comparison code, but I could be convinced. 😇

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 12, 2017

Member

Yes the tag is certainly more readable (although mutable). The reason I chose this solution for now, is that fixing the version-comparison is only half the problem; the CLI would need an update as well, in which case it won't be resolved for older versions (was thinking of comparing by SHA, but printing a user-friendly version).

We need to have a look at it in a wider context though; as I mentioned, with containerd reaching 1.0, there may no longer be a need to do this strict comparison.

FWIW, vendor check is failing, due to #34758 (comment) 😂

Member

thaJeztah commented Dec 12, 2017

Yes the tag is certainly more readable (although mutable). The reason I chose this solution for now, is that fixing the version-comparison is only half the problem; the CLI would need an update as well, in which case it won't be resolved for older versions (was thinking of comparing by SHA, but printing a user-friendly version).

We need to have a look at it in a wider context though; as I mentioned, with containerd reaching 1.0, there may no longer be a need to do this strict comparison.

FWIW, vendor check is failing, due to #34758 (comment) 😂

@vdemeester

LGTM 👼

@dnephin

LGTM

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Dec 12, 2017

Member

I guess #35765 will fix the vendor suite

Member

dnephin commented Dec 12, 2017

I guess #35765 will fix the vendor suite

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 12, 2017

Member

hmf

17:35:40 golang.org/x/oauth2: Err: exit status 128, out: Cloning into '/go/src/github.com/docker/docker/vendor/golang.org/x/oauth2'...
17:35:40 fatal: remote error: internal server error
17:35:40 Build step 'Execute shell' marked build as failure
Member

thaJeztah commented Dec 12, 2017

hmf

17:35:40 golang.org/x/oauth2: Err: exit status 128, out: Cloning into '/go/src/github.com/docker/docker/vendor/golang.org/x/oauth2'...
17:35:40 fatal: remote error: internal server error
17:35:40 Build step 'Execute shell' marked build as failure
Use commit-sha instead of tag for containerd
The `docker info` command compares the installed version
of containerd using a Git-sha. We currently use a tag for
this, but that tag is not returned by the version-API of
containerd, resulting in the `docker info` output to show:

    containerd version: 89623f28b87a6004d4b785663257362d1658a729 (expected: v1.0.0)

This patch changes the `v1.0.0` tag to the commit that
corresponds with the tag, so that the `docker info` output
does not show the `expected:` string.

This should be considered a temporary workaround; the check
for the exact version of containerd that's installed was needed
when we still used the 0.2.x branch, because it did not have
stable releases yet.

With containerd reaching 1.0, and using SemVer, we can likely
do a comparison for "Major" version, or make this a "packaging"
issue, and remove the check entirely (we can still _print_ the
version that's installed if we think it's usefule).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 13, 2017

Member

All tests passed. Merging..

Member

yongtang commented Dec 13, 2017

All tests passed. Merging..

@yongtang yongtang merged commit a03e777 into moby:master Dec 13, 2017

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38356 has succeeded
Details
janky Jenkins build Docker-PRs 47093 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7476 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 4032 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18612 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7298 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:use-commit-instead-of-version branch Dec 13, 2017

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