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

Add external binaries version to docker info #27955

Merged
merged 1 commit into from
Nov 11, 2016
Merged

Add external binaries version to docker info #27955

merged 1 commit into from
Nov 11, 2016

Conversation

mlaventure
Copy link
Contributor

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

--

This should prevent us to having to request that information on new issues.

@mlaventure mlaventure force-pushed the runc-docker-info branch 3 times, most recently from 74042c2 to 9e2fe66 Compare November 1, 2016 18:16
@MHBauer
Copy link
Contributor

MHBauer commented Nov 1, 2016

Example run? Is there an issue? Are we having trouble debugging something specific that this would help?

@mlaventure
Copy link
Contributor Author

@MHBauer

It's mostly for users that are not using docker official packages. Or to spot easy to figure out "error" like them having replaced one of the binaries for testing and forgotten about it.

It's also nice to have it all in one place. It's just information.

LK4D4
LK4D4 reviewed Nov 1, 2016
View reviewed changes
hack/make/.go-autogen
Version string = "$VERSION"
BuildTime string = "$BUILDTIME"
IAmStatic string = "${IAMSTATIC:-true}"
GitCommit string = "7037123-unsupported"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this hardcoded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oups, bad commit -_-

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 1, 2016

I think this should be solved by having --version flag for all binaries.

@mlaventure
Copy link
Contributor Author

@LK4D4 not all users have access to their daemon host

Also, I think having users only run 1 command to get this information is preferable than asking them to run --version on 3 of them.

@thaJeztah
Copy link
Member

from a "support" perspective, I think it'd be great to have it all in docker info, but just my 0.02c (haven't looked at the implementation)

@vdemeester
Copy link
Member

I do agree with @thaJeztah and @mlaventure, having all in docker info makes sence (for remote and also because it's one and only command then 👼)

@krallin krallin mentioned this pull request Nov 3, 2016
Closed
@justincormack
Copy link
Contributor

Needs a rebase

@mlaventure
Add expected 3rd party binaries commit ids to info
2790ac6 
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@tonistiigi
Copy link
Member

LGTM

lowenna
lowenna approved these changes Nov 11, 2016
View reviewed changes
Copy link
Member

@lowenna lowenna left a comment

Choose a reason for hiding this comment

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

Minor nits which can be followed up later. Otherwise LGTM from the Windows side.

daemon/daemon.go
@@ -66,6 +66,9 @@ var (
// containerd if none is specified
DefaultRuntimeBinary = "docker-runc"

// DefaultInitBinary is the name of the default init binary
DefaultInitBinary = "docker-init"
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit, but perhaps this should be in platform files with a comment that it's not used on Windows.

hack/make/.go-autogen
Version string = "$VERSION"
BuildTime string = "$BUILDTIME"
IAmStatic string = "${IAMSTATIC:-true}"
ContainerdCommitID string = "${CONTAINERD_COMMIT}"
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit too - perhaps if not on Windows.

@vieux vieux added this to the 1.13.0 milestone Nov 11, 2016
@vieux vieux mentioned this pull request Nov 11, 2016
Closed
@vieux
Copy link
Contributor

vieux commented Nov 11, 2016

LGTM

@vieux vieux merged commit 0427afa into moby:master Nov 11, 2016
@runcom
Copy link
Member

runcom commented Nov 11, 2016

This is missing docker-proxy

@mlaventure mlaventure deleted the runc-docker-info branch November 11, 2016 15:07
@thaJeztah thaJeztah mentioned this pull request Nov 11, 2016
Merged
@mlaventure
Copy link
Contributor Author

@jhowardmsft I'll make a PR to make those new fields unix only

@runcom it looks like docker-proxy doesn't have a --version or equivalent yet

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
@vieux
Merge pull request moby#27955 from mlaventure/runc-docker-info
19c8941 
Add external binaries version to docker info
@thaJeztah thaJeztah mentioned this pull request May 8, 2024
Merged
This was referenced Sep 10, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LK4D4 LK4D4 LK4D4 left review comments

@lowenna lowenna lowenna approved these changes

Assignees
No one assigned
Labels
impact/changelog impact/cli status/2-code-review
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.