Invert envcmd relationship

Invert envcmd relationship

Previously we embedded EnvCommandBase in all commands
requring an environment, and EnvCommandBase included
everything (SetFlags, and Init). The problem with this
is that it was easy to miss initialisation of the
EnvCommandBase type (this happened a few times), which
leads to bad things happening like sync-tools destroying
environments.

I have inverted the relationship so that we now have
envcmd.EnvironCommand, an interface that extends Command
with a SetEnvName method, and EnvCommandBase, which
implements EnvironCommand. A new method, envcmd.Wrap takes
an EnvironCommand and creates a Command that calls
SetEnvName prior to the wrapped method's Init method. If
the environment name cannot be determined, the wrapping
command will error out early.

https://code.launchpad.net/~axwalk/juju-core/envcmd-inversion/+merge/219135

(do not edit description out of merge proposal)

[axw, 2014-05-12 05:36:37]

Please take a look.

[wallyworld, 2014-05-12 06:33:30]

LGTM with perhaps a 2nd opinion from William.

It does seem that we are missing tests to ensure that each command is wrapped as
expected. We test the wrapping function itself in environmentcommand but I can't
see how or where we are testing that each business command is wrapped eg for
add-machine, how do we test that it has been registered using
jujucmd.Register(wrap(&AddMachineCommand{}))and not just
jujucmd.Register(&AddMachineCommand{})

We should be able to iterate over all registered commands to check. Can we do
this before landing?

[fwereade, 2014-05-12 07:24:18]

This LGTM as far as it goes (modulo the need for testing -- I'd like it to be
impossible to *casually* add a command that isn't an environment command).

But I'm wondering whether it's rational to take it a step further -- do
something like insert the EnvName into the Run args, such that the commands we
create/test/etc are not actually cmd.Commands -- thereby making it impossible(?)
to accidentally register an unwrapped env command.

FWIW, if it were structured that way, I'd be less dogmatic about the need for
testing the list of registered commands...

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

https://codereview.appspot.com/94350045/diff/1/cmd/juju/main.go#newcode140
cmd/juju/main.go:140: jujucmd.Register(wrap(&cmd.VersionCommand{}))
Yeah, as Ian suggested, I'd really like us to be able to directly test that we
actually get functional environment commands -- and ideally we'd get this tested
on *every* registered command.

[axw, 2014-05-12 07:53:07]

On 2014/05/12 07:24:18, fwereade wrote:
> This LGTM as far as it goes (modulo the need for testing -- I'd like it to be
> impossible to *casually* add a command that isn't an environment command).
> 
> But I'm wondering whether it's rational to take it a step further -- do
> something like insert the EnvName into the Run args, such that the commands we
> create/test/etc are not actually cmd.Commands -- thereby making it
impossible(?)
> to accidentally register an unwrapped env command.

Not all commands operate on an environment. I started down this path of having
an alternative Run method (RunInEnv) which takes *cmd.Context and envName, but
found it too clumsy. The approach I've taken takes us in that direction, so it
could be changed with less churn later if desired.

> FWIW, if it were structured that way, I'd be less dogmatic about the need for
> testing the list of registered commands...
> 
> https://codereview.appspot.com/94350045/diff/1/cmd/juju/main.go
> File cmd/juju/main.go (right):
> 
> https://codereview.appspot.com/94350045/diff/1/cmd/juju/main.go#newcode140
> cmd/juju/main.go:140: jujucmd.Register(wrap(&cmd.VersionCommand{}))
> Yeah, as Ian suggested, I'd really like us to be able to directly test that we
> actually get functional environment commands -- and ideally we'd get this
tested
> on *every* registered command.

I'll add a test that checks each registered command is an environment command,
with a list of exceptions.

[axw, 2014-05-12 11:09:21]

On 2014/05/12 07:53:07, axw wrote:
> On 2014/05/12 07:24:18, fwereade wrote:
> > This LGTM as far as it goes (modulo the need for testing -- I'd like it to
be
> > impossible to *casually* add a command that isn't an environment command).
> > 
> > But I'm wondering whether it's rational to take it a step further -- do
> > something like insert the EnvName into the Run args, such that the commands
we
> > create/test/etc are not actually cmd.Commands -- thereby making it
> impossible(?)
> > to accidentally register an unwrapped env command.
> 
> Not all commands operate on an environment. I started down this path of having
> an alternative Run method (RunInEnv) which takes *cmd.Context and envName, but
> found it too clumsy. The approach I've taken takes us in that direction, so it
> could be changed with less churn later if desired.
> 
> > FWIW, if it were structured that way, I'd be less dogmatic about the need
for
> > testing the list of registered commands...
> > 
> > https://codereview.appspot.com/94350045/diff/1/cmd/juju/main.go
> > File cmd/juju/main.go (right):
> > 
> > https://codereview.appspot.com/94350045/diff/1/cmd/juju/main.go#newcode140
> > cmd/juju/main.go:140: jujucmd.Register(wrap(&cmd.VersionCommand{}))
> > Yeah, as Ian suggested, I'd really like us to be able to directly test that
we
> > actually get functional environment commands -- and ideally we'd get this
> tested
> > on *every* registered command.
> 
> I'll add a test that checks each registered command is an environment command,
> with a list of exceptions.

Actually the test is simpler than that: it just checks that there are no
EnvironCommands registered. All EnvironCommands must be wrapped.

[axw, 2014-05-12 11:11:19]

Please take a look.

[wallyworld, 2014-05-12 22:51:04]

On 2014/05/12 11:11:19, axw wrote:
> Please take a look.

LGTM