Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Cleanup add-cloud sub-command #6725
Conversation
| @@ -109,6 +110,9 @@ func (c *AddCloudCommand) Init(args []string) (err error) { | ||
| c.Cloud = args[0] | ||
| } | ||
| if len(args) > 1 { | ||
| + if c.CloudFile != args[1] && c.CloudFile != "" { |
natefinch
Dec 15, 2016
Contributor
this is a little confusing to me. why are we checking cloudfile == args[1] ?
It seems like this would allow you to enter juju add-cloud maas -f foo.yaml foo.yaml which I don't think we should allow. IMO the flag and positional should be mutually exclusive.
seems like we should just check
if len(args) > 1 {
if c.CloudFile != "" {
// return error
}
c.CloudFile = args[1]
}
kat-co
Dec 16, 2016
Contributor
The intent is exactly what you describe: to error if CloudFile has been provided twice, but isn't the same. For justification, please see my earlier comment.
| @@ -499,7 +501,47 @@ func (*addSuite) TestInteractiveAddCloud_PromptForNameIsCorrect(c *gc.C) { | ||
| // Running the command will return an error because we only give | ||
| // enough input to get to the prompt we care about checking. This | ||
| // test ignores this error. |
natefinch
Dec 15, 2016
Contributor
should probably edit the comment to remove the part about ignoring it, since we're not actually ignoring it anymore.
kat-co
Dec 16, 2016
Contributor
Well, we're still ignoring it, I just added some code to ensure we're ignoring the error we think we are :)
| + runCmd := func() { | ||
| + testing.RunCommand(c, command, "garage-maas", "-f", "fake.yaml") | ||
| + } | ||
| + c.Assert(runCmd, gc.PanicMatches, "runtime error: invalid memory address or nil pointer dereference") |
natefinch
Dec 15, 2016
Contributor
Ew. Can we please avoid this? The code is saying that the test should fail if we don't panic... that does not seem to be the right thing to be testing.
We should be able to use InitCommand for this, and then we wouldn't hit the panic, right?
kat-co
Dec 16, 2016
Contributor
The panic comes from the attempt to dereference the nil pointer for the cloudMetadataStore. We could construct an instance and pass it in, but then we'd have to ensure all the endpoints are stubbed out. All of that seemed tangential to what we're trying to test, so this seemed like the best way.
Maybe this is an indication that I'm actually doing more a full-stack test than I want. Maybe I should modify this to test Init is performing correctly. I think I'll do that.
kat-co
Dec 16, 2016
Contributor
Bleh, nope. Can't do that. I don't know how else to check only that the flag is being used properly. I'd sooner delete this test than further complicate it.
| + | ||
| +func (*addSuite) TestSpecifyingCloudFileThroughFlagAndArgument_Errors(c *gc.C) { | ||
| + command := cloud.NewAddCloudCommand(nil) | ||
| + _, err := testing.RunCommand(c, command, "garage-maas", "-f", "fake.yaml", "foo.yaml") |
natefinch
Dec 15, 2016
Contributor
can we do this same test with the two filenames the same? I think that should be an error as well.
kat-co
Dec 16, 2016
Contributor
I disagree. If the filenames are the same, the Juju knows how to continue, and we shouldn't scold the user.
| + ctx := &cmd.Context{ | ||
| + Stdout: &out, | ||
| + Stderr: ioutil.Discard, | ||
| + Stdin: strings.NewReader("" + |
natefinch
Dec 15, 2016
Contributor
can you fix the input so that we successfully run the command entirely? That way if we move the name query later in the sequence, we're still ensured that this test will fail if the name gets queried later.
kat-co
Dec 16, 2016
Contributor
+1. I think we should also consider injecting pollster so we can check for the call explicitly instead of trying to glean what's happening from input/output.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
kat-co commentedDec 15, 2016