Init EnvCommandBase.EnvName from envs.yaml

Init EnvCommandBase.EnvName from envs.yaml

This change brings 1.18 in line with trunk,
where EnvCommandBase.EnvName is initialised
to the default environment if current-environment
does not exist and neither "-e" nor $JUJU_ENV
have been specified.

The primary change is in moving environcommand.go
to cmd/envcmd, and adding an Init method that
reads from environments.yaml as a last resort.
All commands that embed EnvCommandBase are updated
to call this method, and various tests are updated
as a result.

Fixes lp:1316869

https://code.launchpad.net/~axwalk/juju-core/lp1316869-envcmd-from-yaml-1.18/+merge/218745

(do not edit description out of merge proposal)

[axw, 2014-05-08 06:46:30]

Please take a look.

[fwereade, 2014-05-08 07:47:56]

one important comment, will go over the rest in a bit.

https://codereview.appspot.com/92140043/diff/1/cmd/envcmd/environmentcommand.go
File cmd/envcmd/environmentcommand.go (right):

https://codereview.appspot.com/92140043/diff/1/cmd/envcmd/environmentcommand....
cmd/envcmd/environmentcommand.go:73: c.EnvName = envs.Default
I don't think envs.Default is guaranteed to be != "". This method really ought
to fill in EnvName or fail.

BTW, since you're hitting it -- would you please rename it to EnsureEnvName, or
something, so people can tell what it does? And give it a doc comment too,
please. It's *not* the Command.Init method, and the similarity in name causes
more harm than good IMO.

[axw, 2014-05-08 09:35:19]

Please take a look.

https://codereview.appspot.com/92140043/diff/1/cmd/envcmd/environmentcommand.go
File cmd/envcmd/environmentcommand.go (right):

https://codereview.appspot.com/92140043/diff/1/cmd/envcmd/environmentcommand....
cmd/envcmd/environmentcommand.go:73: c.EnvName = envs.Default
On 2014/05/08 07:47:56, fwereade wrote:
> I don't think envs.Default is guaranteed to be != "". This method really ought
> to fill in EnvName or fail.

I've updated the logic to check, and added some tests.

> BTW, since you're hitting it -- would you please rename it to EnsureEnvName,
or
> something, so people can tell what it does? And give it a doc comment too,
> please. It's *not* the Command.Init method, and the similarity in name causes
> more harm than good IMO.

Done. I had shamefully copied from trunk.

[fwereade, 2014-05-08 11:34:11]

I'm happy with the EnsureEnvName implementation, but it STM that we've missed
several places where we should be calling it.

If you fix those and get another review while I sleep, go ahead and land (well,
assuming *that* reviewer is happy)... but (1) there's something wrong with the
approach if it's so easy to get it wrong and (2) the EnsureEnvName fix also
needs to be forward-ported to trunk again.

I'm wondering whether we need to invert it a little bit -- ie we implement an
EnvCommand which embeds an implementation for the bits that do vary, vs
embedding EnvCommandNotReallyBaseAtAll and having to remember to use it right
every time. i.e.

type EnvCommand struct {
    EnvName string
    Behaviour interface {
        SetFlags(f *gnuflag.FlagSet)
        Init(args []string) error
        Run(ctx *cmd.Context, envName string) error
    }
}

func (c *EnvCommand) SetFlags(f *gnuflag.FlagSet) {
    f.StringVar(&c.EnvName, "e", "", "juju environment to operate in ")
    f.StringVar(&c.EnvName, "environment", "", "")
    c.Behaviour.SetFlags(f)
}

func (c *EnvCommand) Init(args []string) error {
    if err := c.EnsureEnvName(); err != nil {
        return err
    }
    return c.Behaviour.Init(args)
}

func (c *EnvCommand) Run(ctx *cmd.Context) error {
    return c.Behaviour.Run(ctx, c.EnvName)
}

Thoughts?

https://codereview.appspot.com/92140043/diff/20001/cmd/envcmd/environmentcomm...
File cmd/envcmd/environmentcommand.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/envcmd/environmentcomm...
cmd/envcmd/environmentcommand.go:90: defaultEnv := getDefaultEnvironment()
hmm, do we need this if we're guaranteed EnsureEnvName? (even if we're not, does
it actually help more than it hinders?)

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go
File cmd/juju/environment.go (left):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go#ol...
cmd/juju/environment.go:129: func (c *SetEnvironmentCommand) Init(args []string)
(err error) {
Isn't this missing an EnsureEnvName()?

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go#ne...
cmd/juju/environment.go:191: envcmd.EnvCommandBase
And this one?

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/expose.go
File cmd/juju/expose.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/expose.go#newcode16
cmd/juju/expose.go:16: envcmd.EnvCommandBase
And this?

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/retryprovisioning.go
File cmd/juju/retryprovisioning.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/retryprovisioning...
cmd/juju/retryprovisioning.go:18: envcmd.EnvCommandBase
And probably this one?

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/ssh.go#newcode29
cmd/juju/ssh.go:29: envcmd.EnvCommandBase
And this?

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/unexpose.go
File cmd/juju/unexpose.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/unexpose.go#newco...
cmd/juju/unexpose.go:16: envcmd.EnvCommandBase
And, indeed, this one.

[axw, 2014-05-09 03:02:56]

On 2014/05/08 11:34:11, fwereade wrote:
> I'm happy with the EnsureEnvName implementation, but it STM that we've missed
> several places where we should be calling it.
> 
> If you fix those and get another review while I sleep, go ahead and land
(well,
> assuming *that* reviewer is happy)... but (1) there's something wrong with the
> approach if it's so easy to get it wrong and (2) the EnsureEnvName fix also
> needs to be forward-ported to trunk again.
> 
> I'm wondering whether we need to invert it a little bit -- ie we implement an
> EnvCommand which embeds an implementation for the bits that do vary, vs
> embedding EnvCommandNotReallyBaseAtAll and having to remember to use it right
> every time. i.e.
> 
> type EnvCommand struct {
>     EnvName string
>     Behaviour interface {
>         SetFlags(f *gnuflag.FlagSet)
>         Init(args []string) error
>         Run(ctx *cmd.Context, envName string) error
>     }
> }
> 
> func (c *EnvCommand) SetFlags(f *gnuflag.FlagSet) {
>     f.StringVar(&c.EnvName, "e", "", "juju environment to operate in ")
>     f.StringVar(&c.EnvName, "environment", "", "")
>     c.Behaviour.SetFlags(f)
> }
> 
> func (c *EnvCommand) Init(args []string) error {
>     if err := c.EnsureEnvName(); err != nil {
>         return err
>     }
>     return c.Behaviour.Init(args)
> }
> 
> func (c *EnvCommand) Run(ctx *cmd.Context) error {
>     return c.Behaviour.Run(ctx, c.EnvName)
> }
> 
> Thoughts?

I agree this should be done, but I'd like to do that on trunk. The only ones
immediately matter are the ones that Prepare, i.e. bootstrap and sync-tools, and
the metadata plugin commands. I've pored over all the commands and I think
they've all been caught now. I'd rather not risk destabilising 1.18 by changing
all the commands around.

>
https://codereview.appspot.com/92140043/diff/20001/cmd/envcmd/environmentcomm...
> File cmd/envcmd/environmentcommand.go (right):
> 
>
https://codereview.appspot.com/92140043/diff/20001/cmd/envcmd/environmentcomm...
> cmd/envcmd/environmentcommand.go:90: defaultEnv := getDefaultEnvironment()
> hmm, do we need this if we're guaranteed EnsureEnvName? (even if we're not,
does
> it actually help more than it hinders?)
> 
> https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go
> File cmd/juju/environment.go (left):
> 
>
https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go#ol...
> cmd/juju/environment.go:129: func (c *SetEnvironmentCommand) Init(args
[]string)
> (err error) {
> Isn't this missing an EnsureEnvName()?
> 
> https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go
> File cmd/juju/environment.go (right):
> 
>
https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go#ne...
> cmd/juju/environment.go:191: envcmd.EnvCommandBase
> And this one?
> 
> https://codereview.appspot.com/92140043/diff/20001/cmd/juju/expose.go
> File cmd/juju/expose.go (right):
> 
>
https://codereview.appspot.com/92140043/diff/20001/cmd/juju/expose.go#newcode16
> cmd/juju/expose.go:16: envcmd.EnvCommandBase
> And this?
> 
>
https://codereview.appspot.com/92140043/diff/20001/cmd/juju/retryprovisioning.go
> File cmd/juju/retryprovisioning.go (right):
> 
>
https://codereview.appspot.com/92140043/diff/20001/cmd/juju/retryprovisioning...
> cmd/juju/retryprovisioning.go:18: envcmd.EnvCommandBase
> And probably this one?
> 
> https://codereview.appspot.com/92140043/diff/20001/cmd/juju/ssh.go
> File cmd/juju/ssh.go (right):
> 
> https://codereview.appspot.com/92140043/diff/20001/cmd/juju/ssh.go#newcode29
> cmd/juju/ssh.go:29: envcmd.EnvCommandBase
> And this?
> 
> https://codereview.appspot.com/92140043/diff/20001/cmd/juju/unexpose.go
> File cmd/juju/unexpose.go (right):
> 
>
https://codereview.appspot.com/92140043/diff/20001/cmd/juju/unexpose.go#newco...
> cmd/juju/unexpose.go:16: envcmd.EnvCommandBase
> And, indeed, this one.

Sorry about that. Fixed them, and a few others as well.

[axw, 2014-05-09 03:12:31]

Please take a look.

https://codereview.appspot.com/92140043/diff/20001/cmd/envcmd/environmentcomm...
File cmd/envcmd/environmentcommand.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/envcmd/environmentcomm...
cmd/envcmd/environmentcommand.go:90: defaultEnv := getDefaultEnvironment()
On 2014/05/08 11:34:10, fwereade wrote:
> hmm, do we need this if we're guaranteed EnsureEnvName? (even if we're not,
does
> it actually help more than it hinders?)

We don't, and we can use "juju env" to get the current environment. It's not
actively hurting anything here, so I'd like to leave it here and remove it in
trunk in case there are some unforeseen consequences.

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go
File cmd/juju/environment.go (left):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go#ol...
cmd/juju/environment.go:129: func (c *SetEnvironmentCommand) Init(args []string)
(err error) {
On 2014/05/08 11:34:10, fwereade wrote:
> Isn't this missing an EnsureEnvName()?

Done.

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/environment.go#ne...
cmd/juju/environment.go:191: envcmd.EnvCommandBase
On 2014/05/08 11:34:10, fwereade wrote:
> And this one?

Done.

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/expose.go
File cmd/juju/expose.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/expose.go#newcode16
cmd/juju/expose.go:16: envcmd.EnvCommandBase
On 2014/05/08 11:34:10, fwereade wrote:
> And this?

Done.

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/retryprovisioning.go
File cmd/juju/retryprovisioning.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/retryprovisioning...
cmd/juju/retryprovisioning.go:18: envcmd.EnvCommandBase
On 2014/05/08 11:34:10, fwereade wrote:
> And probably this one?

Done.

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/ssh.go#newcode29
cmd/juju/ssh.go:29: envcmd.EnvCommandBase
On 2014/05/08 11:34:10, fwereade wrote:
> And this?

Done.

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/unexpose.go
File cmd/juju/unexpose.go (right):

https://codereview.appspot.com/92140043/diff/20001/cmd/juju/unexpose.go#newco...
cmd/juju/unexpose.go:16: envcmd.EnvCommandBase
On 2014/05/08 11:34:10, fwereade wrote:
> And, indeed, this one.

Done.

[wallyworld, 2014-05-09 03:28:59]

LGTM but I agree with William - there's a better approach we can use for trunk.