cmd/juju: working bootstrap and destroy commands

cmd/juju: working bootstrap and destroy commands



https://code.launchpad.net/~rogpeppe/juju/go-more-commands/+merge/103532

Requires: https://code.launchpad.net/~rogpeppe/juju/go-dummy-broken-flag/+merge/103528

(do not edit description out of merge proposal)

[rog, 2012-04-25 17:59:39]

see also https://codereview.appspot.com/5901058
which is a previous version of this CL with a different
pre-requisite.

[rog, 2012-04-26 11:32:27]

Please take a look.

[rog, 2012-04-26 11:34:38]

Please take a look.

[TheMue, 2012-04-26 11:44:47]

Only minor comments about the placement of test data.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go#newcode96
cmd/juju/cmd_test.go:96: // All members of genericTests are tested for the
-environment and -e
If this data is only used in TestEnvironmentInit() I would like to see it
declared there.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go#newcode124
cmd/juju/cmd_test.go:124: var cmdTests = []struct {
See above.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main_test.go#newcod...
cmd/juju/main_test.go:127: var commandNames = []string{
Would like to see this inside of TestHelp().

[rog, 2012-04-26 12:10:26]

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go#newcode96
cmd/juju/cmd_test.go:96: // All members of genericTests are tested for the
-environment and -e
On 2012/04/26 11:44:47, TheMue wrote:
> If this data is only used in TestEnvironmentInit() I would like to see it
> declared there.

sorry, but it's conventional for tables to live outside the functions (just like
we don't nest function definitions);
just check the main Go tree for examples.

that said, the name could probably follow the name of the test, to make the
association more obvious.

EnvironmentInitTests, for example. i'll do that, if that's ok.

[fwereade, 2012-04-26 12:28:25]

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/bootstrap.go#newcode12
cmd/juju/bootstrap.go:12: Conn *juju.Conn
Consider adding a type used by both Bootstrap and Destroy to contribute flags in
Init and convert them to a Conn in Run. (see cmd.Log in
lp:~fwereade/juju/go-extract-cmd-log)

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go#newcode96
cmd/juju/cmd_test.go:96: // All members of genericTests are tested for the
-environment and -e
On 2012/04/26 12:10:27, rog wrote:
> On 2012/04/26 11:44:47, TheMue wrote:
> > If this data is only used in TestEnvironmentInit() I would like to see it
> > declared there.
> 
> sorry, but it's conventional for tables to live outside the functions (just
like
> we don't nest function definitions);
> just check the main Go tree for examples.
> 
> that said, the name could probably follow the name of the test, to make the
> association more obvious.
> 
> EnvironmentInitTests, for example. i'll do that, if that's ok.

Shame... I've come to feel that there's something very pleasing and
self-contained about this style:

  for t := range []struct{
      foo string
      bar string
  } {
      {"ping", "pong"},
      {"death", "star"},
  }{
      // tests
  }

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go#newcode118
cmd/juju/cmd_test.go:118: testInit(c, command, []string{"-e", "unknown"},
"unknown environment .*")
I think this error should be coming out of Run, really.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/destroy.go
File cmd/juju/destroy.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/destroy.go#newcode11
cmd/juju/destroy.go:11: Conn *juju.Conn
As BootstrapCommand

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main.go#newcode8
cmd/juju/main.go:8: // Environment types to include.
This feels a bit magical. Bit more explanation?

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main_test.go#newcode97
cmd/juju/main_test.go:97: msg := "broken environment"
Nice.

[rog, 2012-04-26 13:12:07]

Please take a look.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/bootstrap.go#newcode12
cmd/juju/bootstrap.go:12: Conn *juju.Conn
On 2012/04/26 12:28:25, fwereade wrote:
> Consider adding a type used by both Bootstrap and Destroy to contribute flags
in
> Init and convert them to a Conn in Run. (see cmd.Log in
> lp:~fwereade/juju/go-extract-cmd-log)

I started by making a type, but it felt like overkill. Try this for size.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go#newcode96
cmd/juju/cmd_test.go:96: // All members of genericTests are tested for the
-environment and -e
On 2012/04/26 12:28:25, fwereade wrote:
> On 2012/04/26 12:10:27, rog wrote:
> > On 2012/04/26 11:44:47, TheMue wrote:
> > > If this data is only used in TestEnvironmentInit() I would like to see it
> > > declared there.
> > 
> > sorry, but it's conventional for tables to live outside the functions (just
> like
> > we don't nest function definitions);
> > just check the main Go tree for examples.
> > 
> > that said, the name could probably follow the name of the test, to make the
> > association more obvious.
> > 
> > EnvironmentInitTests, for example. i'll do that, if that's ok.
> 
> Shame... I've come to feel that there's something very pleasing and
> self-contained about this style:
> 
>   for t := range []struct{
>       foo string
>       bar string
>   } {
>       {"ping", "pong"},
>       {"death", "star"},
>   }{
>       // tests
>   }

i prefer the top of the loop to be near its body. and i like the fact that the
data is obviously static because it's global.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/cmd_test.go#newcode118
cmd/juju/cmd_test.go:118: testInit(c, command, []string{"-e", "unknown"},
"unknown environment .*")
On 2012/04/26 12:28:25, fwereade wrote:
> I think this error should be coming out of Run, really.

Done.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/destroy.go
File cmd/juju/destroy.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/destroy.go#newcode11
cmd/juju/destroy.go:11: Conn *juju.Conn
On 2012/04/26 12:28:25, fwereade wrote:
> As BootstrapCommand

Done.

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/6120052/diff/8001/cmd/juju/main.go#newcode8
cmd/juju/main.go:8: // Environment types to include.
On 2012/04/26 12:28:25, fwereade wrote:
> This feels a bit magical. Bit more explanation?

Done.

[fwereade, 2012-04-26 13:46:32]

LGTM modulo query below

https://codereview.appspot.com/6120052/diff/6003/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (left):

https://codereview.appspot.com/6120052/diff/6003/cmd/juju/bootstrap.go#oldcode31
cmd/juju/bootstrap.go:31: return cmd.CheckEmpty(f.Args())
What's wrong with the shorter form?

[rog, 2012-04-26 15:43:34]

Please take a look.

https://codereview.appspot.com/6120052/diff/6003/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (left):

https://codereview.appspot.com/6120052/diff/6003/cmd/juju/bootstrap.go#oldcode31
cmd/juju/bootstrap.go:31: return cmd.CheckEmpty(f.Args())
On 2012/04/26 13:46:32, fwereade wrote:
> What's wrong with the shorter form?

my brain.

[rog, 2012-04-26 16:14:59]

Please take a look.

[rog, 2012-04-26 16:21:16]

Please take a look.

[rog, 2012-04-26 16:54:20]

Please take a look.

[rog, 2012-04-26 16:55:34]

Please take a look.

[niemeyer, 2012-04-26 18:35:16]

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/cmd_test.go#newcode112
cmd/juju/cmd_test.go:112: func runCommand(com cmd.Command, args ...string) (opc
chan dummy.Operation, errc chan error) {
That's a lot nicer, thanks. This might even make the table design feel better,
at least for error conditions I suppose.

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/cmd_test.go#newcode138
cmd/juju/cmd_test.go:138: // bootstrap with broken environment
So, that's what I was referring to in the other CL. What is this actually
testing? This test makes every single method in the dummy environment explode,
and then check that *something* verified an err return so that it pops up. I'd
like to hear your ideas on this as it feels to me like an unclear strategy with
smoke and mirrors.

[niemeyer, 2012-04-26 18:38:34]

Sorry, published too soon.

There's one trivial, and LGTM, even with the test question unsorted (happy to
talk about it online later).

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/destroy.go
File cmd/juju/destroy.go (right):

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/destroy.go#newcode27
cmd/juju/destroy.go:27: if err := cmd.CheckEmpty(f.Args()); err != nil {
return cmd.CheckEmpty(f.Args())

[rog, 2012-04-27 08:28:03]

*** Submitted:

cmd/juju: working bootstrap and destroy commands

R=TheMue, fwereade, niemeyer
CC=
https://codereview.appspot.com/6120052

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/cmd_test.go#newcode112
cmd/juju/cmd_test.go:112: func runCommand(com cmd.Command, args ...string) (opc
chan dummy.Operation, errc chan error) {
On 2012/04/26 18:35:16, niemeyer wrote:
> That's a lot nicer, thanks. This might even make the table design feel better,
> at least for error conditions I suppose.

Thanks.
I think I've come around to the idea that table-based isn't great for this
particular test. When we get to the test for client upload, we'll see why, I
believe.

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/cmd_test.go#newcode138
cmd/juju/cmd_test.go:138: // bootstrap with broken environment
On 2012/04/26 18:35:16, niemeyer wrote:
> So, that's what I was referring to in the other CL. What is this actually
> testing? This test makes every single method in the dummy environment explode,
> and then check that *something* verified an err return so that it pops up. I'd
> like to hear your ideas on this as it feels to me like an unclear strategy
with
> smoke and mirrors.

The idea is to test the error path. I wonder if it would be better if the dummy
environment did register the operation as well as returning the "broken" error.
Then we'd be checking no only that the correct call was made, but also that the
error path worked. We'll talk about it later.

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/destroy.go
File cmd/juju/destroy.go (right):

https://codereview.appspot.com/6120052/diff/9013/cmd/juju/destroy.go#newcode27
cmd/juju/destroy.go:27: if err := cmd.CheckEmpty(f.Args()); err != nil {
On 2012/04/26 18:38:34, niemeyer wrote:
> return cmd.CheckEmpty(f.Args())

Done.