Include whether the managers in the swarm are autolocked as part of docker info - #471
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 |
dnephin
left a comment
dnephin
left a comment
There was a problem hiding this comment.
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.
| 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.
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
| 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.
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.
| 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.
Using the fake this would be assert.Equal(t, "", cli.ErrBuffer().String())
| require.Empty(t, result) | ||
|
|
||
| // with swarm | ||
| cli, stdout, stderr = newDockerCLI() |
There was a problem hiding this comment.
Please make this a separate test function TestPrettyPrintInfoWithSwarm() which removes the need for this comment
| return command.NewDockerCli(ioutil.NopCloser(&in), &out, &err), &out, &err | ||
| } | ||
|
|
||
| func TestInfoNoWarnings(t *testing.T) { |
There was a problem hiding this comment.
Following our testing guidelines (which we just merged), this should be TestPrettyPrintInfoNoSwarm()
| infoWithSwarm := sampleInfoNoSwarm | ||
| infoWithSwarm.Swarm = sampleSwarmInfo | ||
| expected := []byte( | ||
| strings.Replace(sampleTextOutputNoSwarm, "Swarm: inactive", sampleTextOutputJustSwarm, 1)) |
There was a problem hiding this comment.
I would just write out the exact file and compare using golden instead of doing a replace
| 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.
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. |
6 participants
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