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.

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

Separate client infomation in docker system info #1638

Merged
merged 5 commits into from
Jan 25, 2019

Conversation

ijc
Copy link
Contributor

@ijc ijc commented Jan 24, 2019

- What I did

Split the output of docker info (aka docker system info) into client and server sections.

Currently there is only one client setting (debug), but this arose from the discussion at #1564 which wants to add info about cli plugins.

The new arrangements are also to print as much as possible of both the client and server information even in the presence of errors (e.g. gathering the info or rendering some part of it) and to list those errors at the end (and return an overall error).

In the non-pretty case (--format='{{json .}}' etc) the daemon info remains at the top-level (for API backwards compatibility) while the client info is in a new ClientInfo key. Ideally the daemon info would be in a DaemonInfo key for consistency but that would break the format which I presume is considered stable and which people rely on (if not I should definitely make it cleaner). Errors are given in new ClientErrors and DaemonErrors keys. Not that things would go a bit strange if fields with the same names were added to the daemon types.Info. Given the names in question I think/hope that's unlikely to happen.

Note that the client debug setting was not previously in the JSON (which was purely daemon info) despite being in the pretty version.

- How I did it

Hard graft.

- How to verify it

I added and extended the units tests (including pulling some useful/relevant bits from #1564). You could also just try docker info with and without a valid daemon.

- Description for the changelog

The docker system info output now segregates information relevant to the client and daemon.

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

@codecov-io
Copy link

codecov-io commented Jan 24, 2019

Codecov Report

Merging #1638 into master will decrease coverage by 0.39%.
The diff coverage is 78.62%.

@@            Coverage Diff            @@
##           master    #1638     +/-   ##
=========================================
- Coverage   55.62%   55.22%   -0.4%     
=========================================
  Files         301      301             
  Lines       20444    20410     -34     
=========================================
- Hits        11372    11272    -100     
- Misses       8253     8332     +79     
+ Partials      819      806     -13

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! Overall looks good; I left a comment about the format, and some minor nits 🤗

cli/command/system/info_test.go Outdated Show resolved Hide resolved
Debug Mode: true

Daemon
------
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a separator-line, should we use the same format as on docker version?

Client: Docker Engine - Community
  Debug Mode: true

Server: Docker Engine - Community
  Containers: 0
    Running: 0
    Paused: 0

....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just trying to avoid touching every single line ;-) (there's also a small possibility of breaking scripts, but they should be using the machine readable form anyway).

I'll make the indentation change and look into adding the platform version -- not sure how tricky that is to get hold of here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I noticed that this output doesn't use a template or a tab-writer, so unfortunately it's a bit more labor intensive (something we should change after this to be more flexible)

Sorry! 🤗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thaJeztah I reformatted and added the space (in a separate commit since it is rather noisy -- hopefully GH diff will render it ok for review).

I didn't add the Docker Engine - Foo bits, since they are already in docker version, there is already a Product License: Community Engine in the daemon stuff and I wasn't sure how best to include the info in the JSON anyhow. Hope that's ok! Shout if not and I'll have another stab at it.

docs/reference/commandline/info.md Outdated Show resolved Hide resolved
docs/reference/commandline/info.md Outdated Show resolved Hide resolved
docs/reference/commandline/info.md Outdated Show resolved Hide resolved
man/src/system/info.md Outdated Show resolved Hide resolved
cli/command/system/info_test.go Outdated Show resolved Hide resolved
Ian Campbell added 2 commits January 25, 2019 11:55
Signed-off-by: Ian Campbell <ijc@docker.com>
This is in addition to the more specific `formatInfo` unit tests added
previously.

Signed-off-by: Ian Campbell <ijc@docker.com>
@ijc ijc force-pushed the separate-client-system-info branch from a602486 to 9145a51 Compare January 25, 2019 12:09
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM after s/Daemon:/Server:. Thanks!!

cli/command/system/info.go Outdated Show resolved Hide resolved
Ian Campbell added 3 commits January 25, 2019 14:38
Right now the only client side info we have is whether debug is enabled, but we
expect more in the future.

We also preemptively prepare for the possibility of multiple errors when
gathering both daemon and client info.

Signed-off-by: Ian Campbell <ijc@docker.com>
This allows it to print what it can, rather than aborting half way when a bad
security context is hit.

Signed-off-by: Ian Campbell <ijc@docker.com>
That is, reindent the two sections by one space.

While the code was done by hand the `.golden` files had the extra space
inserted with emacs' `string-insert-rectangle` macro to (try to) avoid possible
manual errors. The docs were edited the same way.

Signed-off-by: Ian Campbell <ijc@docker.com>
@ijc ijc force-pushed the separate-client-system-info branch from c1af7fc to bcb06b5 Compare January 25, 2019 14:46
@ijc
Copy link
Contributor Author

ijc commented Jan 25, 2019

@thaJeztah made that s/Daemon/Server/g change, thanks!

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM! 🤗

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @ijc 👍

@thaJeztah
Copy link
Member

Lets Get This Merged

@thaJeztah thaJeztah merged commit 080f30a into docker:master Jan 25, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Jan 25, 2019
@ijc ijc deleted the separate-client-system-info branch January 25, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants