-
Notifications
You must be signed in to change notification settings - Fork 2k
Include whether the managers in the swarm are autolocked as part of docker info
#471
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #471 +/- ##
==========================================
+ Coverage 47.39% 48.54% +1.14%
==========================================
Files 199 199
Lines 16405 16410 +5
==========================================
+ Hits 7775 7966 +191
+ Misses 8233 8030 -203
- Partials 397 414 +17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! If the data is already in Info then I think it does make sense to print it.
A few comments about the tests to make them more readable and consistent with our existing tests.
cli/command/system/info_test.go
Outdated
| func newDockerCLI() (*command.DockerCli, *bytes.Buffer, *bytes.Buffer) { | ||
| var in, out, err bytes.Buffer | ||
| return command.NewDockerCli(ioutil.NopCloser(&in), &out, &err), &out, &err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a fake that we use for testing at internal/test/ NewFakeCli()
You can change prettyPrintInfo() to accept a the command.Cli interface instead of the struct
cli/command/system/info_test.go
Outdated
| require.NoError(t, prettyPrintInfo(cli, sampleInfoNoSwarm)) | ||
| result, err := ioutil.ReadAll(stdout) | ||
| require.NoError(t, err) | ||
| require.Equal(t, []byte(sampleTextOutputNoSwarm), result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend moving these sample texts to "golden files" (files in ./testdata relative to this package directory) and using gotestyourself/golden.Assert() to compare them. There are a bunch of example in this repo and the godoc is here: https://godoc.org/github.com/gotestyourself/gotestyourself/golden
This will make it easier to update and maintain the expected result, and it also uses difflib to give you a nice multiline diff when there are failures as long as you use the string version (not bytes). Comparing bytes gives you a diff of an array of uint8 which is impossible to debug.
cli/command/system/info_test.go
Outdated
| require.Equal(t, []byte(sampleTextOutputNoSwarm), result) | ||
| result, err = ioutil.ReadAll(stderr) | ||
| require.NoError(t, err) | ||
| require.Empty(t, result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the fake this would be assert.Equal(t, "", cli.ErrBuffer().String())
cli/command/system/info_test.go
Outdated
| require.Empty(t, result) | ||
|
|
||
| // with swarm | ||
| cli, stdout, stderr = newDockerCLI() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a separate test function TestPrettyPrintInfoWithSwarm() which removes the need for this comment
cli/command/system/info_test.go
Outdated
| return command.NewDockerCli(ioutil.NopCloser(&in), &out, &err), &out, &err | ||
| } | ||
|
|
||
| func TestInfoNoWarnings(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following our testing guidelines (which we just merged), this should be TestPrettyPrintInfoNoSwarm()
cli/command/system/info_test.go
Outdated
| infoWithSwarm := sampleInfoNoSwarm | ||
| infoWithSwarm.Swarm = sampleSwarmInfo | ||
| expected := []byte( | ||
| strings.Replace(sampleTextOutputNoSwarm, "Swarm: inactive", sampleTextOutputJustSwarm, 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just write out the exact file and compare using golden instead of doing a replace
cli/command/system/info.go
Outdated
| u := dockerCli.ConfigFile().AuthConfigs[info.IndexServerAddress].Username | ||
| if len(u) > 0 { | ||
| fmt.Fprintf(dockerCli.Out(), "Username: %v\n", u) | ||
| if dockerCli.ConfigFile() != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is just because of the test, I think you can revert this. Using the FakeCli should give you a default config file I believe.
…docker info`. Signed-off-by: Ying Li <ying.li@docker.com>
|
Thanks for the review @dnephin! I think I've addressed all the comments, and added a test for the warnings since the golden testdata files makes that way easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐮

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

Currently, the only way to tell if the cluster is autolocked is to attempt to unlock or get the unlock key. The info is already available on the API for
docker info, so also include it in the text output on the CLI.cc @friism