cmd/juju: working bootstrap and destroy commands

cmd/juju: working bootstrap and destroy commands



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

Requires: https://code.launchpad.net/~rogpeppe/juju/go-testing-environs/+merge/99050

(do not edit description out of merge proposal)

[rog, 2012-03-26 11:52:14]

Please take a look.

[TheMue, 2012-03-26 12:02:16]

Two small comments.

https://codereview.appspot.com/5901058/diff/1011/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/5901058/diff/1011/cmd/command.go#newcode62
cmd/command.go:62: // PrintUsage prints usage information for c to stderr.
I would expect the output on stout, because it's no error. Additionally piping
to more, grep, etc. is simpler then.

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

https://codereview.appspot.com/5901058/diff/1011/cmd/juju/destroy.go#newcode15
cmd/juju/destroy.go:15: "start up an environment from scratch",
The purpose sounds a bit strange related to "destroy".

[rog, 2012-03-26 12:28:36]

Please take a look.

https://codereview.appspot.com/5901058/diff/1011/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/5901058/diff/1011/cmd/command.go#newcode62
cmd/command.go:62: // PrintUsage prints usage information for c to stderr.
On 2012/03/26 12:02:16, TheMue wrote:
> I would expect the output on stout, because it's no error. Additionally piping
> to more, grep, etc. is simpler then.

whether it's an error depends on why it's being printed. the current standard
for Go commands is to print to stderr even when -help is explicitly mentioned.
if we want it different, we'll have to change the flag API.

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

https://codereview.appspot.com/5901058/diff/1011/cmd/juju/destroy.go#newcode15
cmd/juju/destroy.go:15: "start up an environment from scratch",
On 2012/03/26 12:02:16, TheMue wrote:
> The purpose sounds a bit strange related to "destroy".

yes indeed!
done.

[TheMue, 2012-03-26 12:30:29]

LGTM

[niemeyer, 2012-03-26 23:10:48]

Looks quite nice.

https://codereview.appspot.com/5901058/diff/4001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/5901058/diff/4001/cmd/command.go#newcode41
cmd/command.go:41: // InitFlagSet prepares a FlagSet such that Parsing that
FlagSet will
Either "parsing" or "calling Parse on" would be better, otherwise it's a
misreference.

https://codereview.appspot.com/5901058/diff/4001/cmd/juju/flag.go
File cmd/juju/flag.go (right):

https://codereview.appspot.com/5901058/diff/4001/cmd/juju/flag.go#newcode18
cmd/juju/flag.go:18: f.StringVar(&c.envName, "e", "", "juju environment to
operate in")
This type and file don't seem to pull their weight. It obscures the logic by
introducing an indirection that doesn't really save much.

Can we please wait for these commands to grow a bit more before attempting to
deduce abstractions like that?  We'll be in a better position to tell how to
actually save duplication by then.

https://codereview.appspot.com/5901058/diff/4001/cmd/juju/flag.go#newcode24
cmd/juju/flag.go:24: c.Conn, err = juju.NewConn(c.envName)
Ditto:

    err := c.conn.InitConn()

vs.

    c.conn, err = juju.NewConn(c.envName)

It's just not worth it. Forces the developer to look around for logic and
location of data for very little gain.

[rog, 2012-03-27 14:14:05]

Please take a look.

[rog, 2012-03-27 14:14:55]

Please take a look.

[rog, 2012-03-28 16:38:28]

Please take a look.

https://codereview.appspot.com/5901058/diff/4001/cmd/command.go
File cmd/command.go (right):

https://codereview.appspot.com/5901058/diff/4001/cmd/command.go#newcode41
cmd/command.go:41: // InitFlagSet prepares a FlagSet such that Parsing that
FlagSet will
On 2012/03/26 23:10:48, niemeyer wrote:
> Either "parsing" or "calling Parse on" would be better, otherwise it's a
> misreference.

Done.

https://codereview.appspot.com/5901058/diff/4001/cmd/juju/flag.go
File cmd/juju/flag.go (right):

https://codereview.appspot.com/5901058/diff/4001/cmd/juju/flag.go#newcode18
cmd/juju/flag.go:18: f.StringVar(&c.envName, "e", "", "juju environment to
operate in")
On 2012/03/26 23:10:48, niemeyer wrote:
> This type and file don't seem to pull their weight. It obscures the logic by
> introducing an indirection that doesn't really save much.
> 
> Can we please wait for these commands to grow a bit more before attempting to
> deduce abstractions like that?  We'll be in a better position to tell how to
> actually save duplication by then.

every command that requires an environment (that's all of them currently, i
think) will duplicate this code. i know it's not much, but it means we don't
have n different occurrences of the string "juju environment to operate in", and
it simplifies the tests because we can assume that if a command implements one
the required environment semantics it implements all of them.

that said, i appreciate the point of view.
i'll axe it.

[niemeyer, 2012-04-11 01:06:24]

LGTM, but I suggest cleaning those tests.

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

https://codereview.appspot.com/5901058/diff/13013/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:153: c.Assert(err, ErrorMatches, t.parseErr)
s/parseErr/runErr/?

The amount of generality in this file feels a bit over the top, by the way.
There's a lot of unused fields and logic, and several helpers with reflection
and whatnot, and the actual tests being done are trivial. I'd prefer to go
towards a more trivial testing approach that can actually be read and reasoned
about, than tables full of flags and magic as we have here. It's fine to leave
non-existent tests out of the logic entirely.

https://codereview.appspot.com/5901058/diff/13013/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/5901058/diff/13013/environs/dummy/environs.go#...
environs/dummy/environs.go:125: "broken": schema.Bool(),
gofmt