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

Include whether the managers in the swarm are autolocked as part of docker info #471

Merged
merged 1 commit into from
Aug 28, 2017
Merged

Conversation

cyli
Copy link
Contributor

@cyli cyli commented Aug 24, 2017

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

image

@codecov-io
Copy link

codecov-io commented Aug 24, 2017

Codecov Report

Merging #471 into master will increase coverage by 1.14%.
The diff coverage is 100%.

@@            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

Copy link
Contributor

@dnephin dnephin left a 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.

func newDockerCLI() (*command.DockerCli, *bytes.Buffer, *bytes.Buffer) {
var in, out, err bytes.Buffer
return command.NewDockerCli(ioutil.NopCloser(&in), &out, &err), &out, &err
}
Copy link
Contributor

@dnephin dnephin Aug 25, 2017

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

require.NoError(t, prettyPrintInfo(cli, sampleInfoNoSwarm))
result, err := ioutil.ReadAll(stdout)
require.NoError(t, err)
require.Equal(t, []byte(sampleTextOutputNoSwarm), result)
Copy link
Contributor

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.

require.Equal(t, []byte(sampleTextOutputNoSwarm), result)
result, err = ioutil.ReadAll(stderr)
require.NoError(t, err)
require.Empty(t, result)
Copy link
Contributor

@dnephin dnephin Aug 25, 2017

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())

require.Empty(t, result)

// with swarm
cli, stdout, stderr = newDockerCLI()
Copy link
Contributor

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

return command.NewDockerCli(ioutil.NopCloser(&in), &out, &err), &out, &err
}

func TestInfoNoWarnings(t *testing.T) {
Copy link
Contributor

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()

infoWithSwarm := sampleInfoNoSwarm
infoWithSwarm.Swarm = sampleSwarmInfo
expected := []byte(
strings.Replace(sampleTextOutputNoSwarm, "Swarm: inactive", sampleTextOutputJustSwarm, 1))
Copy link
Contributor

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

u := dockerCli.ConfigFile().AuthConfigs[info.IndexServerAddress].Username
if len(u) > 0 {
fmt.Fprintf(dockerCli.Out(), "Username: %v\n", u)
if dockerCli.ConfigFile() != nil {
Copy link
Contributor

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>
@cyli
Copy link
Contributor Author

cyli commented Aug 25, 2017

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.

Copy link
Contributor

@dnephin dnephin 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
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐮

@vdemeester vdemeester merged commit 7e52344 into docker:master Aug 28, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 28, 2017
@cyli cyli deleted the surface-autolock branch August 28, 2017 18:38
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.

6 participants