cmd/juju: warn about existing jenv file

cmd/juju: warn about existing jenv file

Warn the user when using an existing .jenv file instead of
environments.yaml.

Mitigates issue lp:1282642

https://code.launchpad.net/~mfoord/juju-core/jenv-warning/+merge/209421

(do not edit description out of merge proposal)

[fuzzyman, 2014-03-05 14:56:02]

Please take a look.

[jameinel, 2014-03-05 15:01:45]

LGTM with a tweak to the message

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

https://codereview.appspot.com/71490045/diff/1/cmd/juju/bootstrap.go#newcode118
cmd/juju/bootstrap.go:118: logger.Warningf("Found and using existing .jenv
file")
I think this would be easier for users as:

"Using existing environment file: /path/to/.juju/environments/foo.jenv"

[fuzzyman, 2014-03-05 15:48:17]

Please take a look.

[fuzzyman, 2014-03-05 16:43:01]

Please take a look.

[fuzzyman, 2014-03-06 11:31:21]

Please take a look.

[dimitern, 2014-03-06 11:37:23]

LGTM with two minor suggestions.

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/disk....
environs/configstore/disk.go:162: // Location returns the location of the
environInfo in human readable format
s/format/format./

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/inter...
File environs/configstore/interface.go (right):

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/inter...
environs/configstore/interface.go:78: // information in a human readable format
s/format/format./ ?

[fuzzyman, 2014-03-06 11:48:33]

Please take a look.

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/disk....
environs/configstore/disk.go:162: // Location returns the location of the
environInfo in human readable format
On 2014/03/06 11:37:24, dimitern wrote:
> s/format/format./

Done.

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/inter...
File environs/configstore/interface.go (right):

https://codereview.appspot.com/71490045/diff/40001/environs/configstore/inter...
environs/configstore/interface.go:78: // information in a human readable format
On 2014/03/06 11:37:24, dimitern wrote:
> s/format/format./ ?

Done.

[rog, 2014-03-06 12:14:58]

LGTM with a couple of suggestions below.

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

https://codereview.appspot.com/71490045/diff/40001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:119: logger.Warningf("using existing environment file:
%q", loc)
with a slightly shorter variable name, it perhaps seems
more reasonable inline here. Also, given that Location
is documented to be human readable, perhaps we don't
need to quote it.

logger.Warningf("using existing environment at %s", info.Location())

?

https://codereview.appspot.com/71490045/diff/40001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/71490045/diff/40001/cmd/juju/bootstrap_test.go...
cmd/juju/bootstrap_test.go:338: c.Assert((<-opc).(dummy.OpPutFile).Env,
gc.Equals, "peckham")
I don't think you need this (and it makes the test slightly more fragile)

[mfoord, 2014-03-06 12:19:38]

On 2014/03/06 12:14:58, rog wrote:
> LGTM with a couple of suggestions below.
> 
> https://codereview.appspot.com/71490045/diff/40001/cmd/juju/bootstrap.go
> File cmd/juju/bootstrap.go (right):
> 
>
https://codereview.appspot.com/71490045/diff/40001/cmd/juju/bootstrap.go#newc...
> cmd/juju/bootstrap.go:119: logger.Warningf("using existing environment file:
> %q", loc)
> with a slightly shorter variable name, it perhaps seems
> more reasonable inline here. 

I don't know what this means I'm afraid.

> Also, given that Location
> is documented to be human readable, perhaps we don't
> need to quote it.
> 
> logger.Warningf("using existing environment at %s", info.Location())
> 

Well, I don't know that how Location() is *documented* should affect how we
treat it. Either the quotes are helpful or they aren't. For copying and pasting
to the shell they *may* be useful (the location could contain spaces), so I'm
inclined to leave them.

> ?
> 
> https://codereview.appspot.com/71490045/diff/40001/cmd/juju/bootstrap_test.go
> File cmd/juju/bootstrap_test.go (right):
> 
>
https://codereview.appspot.com/71490045/diff/40001/cmd/juju/bootstrap_test.go...
> cmd/juju/bootstrap_test.go:338: c.Assert((<-opc).(dummy.OpPutFile).Env,
> gc.Equals, "peckham")
> I don't think you need this (and it makes the test slightly more fragile)

We need to read from *one* of the channels returned by runCommand, or the
command isn't actually run. It maybe that just reading the err channel is
enough, in which case this assert is superfluous. I'll check.

[fuzzyman, 2014-03-06 14:40:47]

Please take a look.

[fuzzyman, 2014-03-06 14:56:33]

Please take a look.

[fwereade, 2014-03-06 15:17:19]

a few comments, but not really enough to block it. thoughts though?

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

https://codereview.appspot.com/71490045/diff/100001/cmd/juju/bootstrap.go#new...
cmd/juju/bootstrap.go:119: logger.Warningf("using existing environment file:
%q", loc)
perhaps "ignoring environments.yaml: using bootstrap config in %q"

https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk...
environs/configstore/disk.go:163: func (info *environInfo) Location() string {
Path()?

https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk...
File environs/configstore/disk_test.go (right):

https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk...
environs/configstore/disk_test.go:113: c.Assert(info.Location(), gc.Matches,
".*/someenv.jenv")
if we know the full path, which we should, I'd really like to assert it
specifically

https://codereview.appspot.com/71490045/diff/100001/environs/configstore/mem.go
File environs/configstore/mem.go (right):

https://codereview.appspot.com/71490045/diff/100001/environs/configstore/mem....
environs/configstore/mem.go:76: return "memory"
ah. hmm. I see. I think I'd really rather have Path() (string, error); return
("", ErrNotADiskStore); and handle that error in the command.

overkill?

[wwitzel3, 2014-03-06 15:36:12]

On 2014/03/06 15:17:19, fwereade wrote:
> a few comments, but not really enough to block it. thoughts though?
> 
> https://codereview.appspot.com/71490045/diff/100001/cmd/juju/bootstrap.go
> File cmd/juju/bootstrap.go (right):
> 
>
https://codereview.appspot.com/71490045/diff/100001/cmd/juju/bootstrap.go#new...
> cmd/juju/bootstrap.go:119: logger.Warningf("using existing environment file:
> %q", loc)
> perhaps "ignoring environments.yaml: using bootstrap config in %q"
> 
Updated message to the user and updated Location to return a quoted string.

>
https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk.go
> File environs/configstore/disk.go (right):
> 
>
https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk...
> environs/configstore/disk.go:163: func (info *environInfo) Location() string {
> Path()?
> 
>
https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk...
> File environs/configstore/disk_test.go (right):
> 
>
https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk...
> environs/configstore/disk_test.go:113: c.Assert(info.Location(), gc.Matches,
> ".*/someenv.jenv")
> if we know the full path, which we should, I'd really like to assert it
> specifically

Updated test to use full path.

> 
>
https://codereview.appspot.com/71490045/diff/100001/environs/configstore/mem.go
> File environs/configstore/mem.go (right):
> 
>
https://codereview.appspot.com/71490045/diff/100001/environs/configstore/mem....
> environs/configstore/mem.go:76: return "memory"
> ah. hmm. I see. I think I'd really rather have Path() (string, error); return
> ("", ErrNotADiskStore); and handle that error in the command.
> 
> overkill?

[fuzzyman, 2014-03-06 15:37:19]

Please take a look.

[wwitzel3, 2014-03-06 15:52:59]

On 2014/03/06 15:37:19, fuzzyman wrote:
> Please take a look.

LGMT

[fuzzyman, 2014-03-07 09:52:30]

Please take a look.

[rog, 2014-03-07 09:56:27]

On 2014/03/07 09:52:30, fuzzyman wrote:
> Please take a look.

still LGTM