Code review comment for lp:~mfoord/juju-core/jenv-warning

Revision history for this message
William Reade (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#newcode119
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.go#newcode163
environs/configstore/disk.go:163: func (info *environInfo) Location()
string {
Path()?

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

https://codereview.appspot.com/71490045/diff/100001/environs/configstore/disk_test.go#newcode113
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.go#newcode76
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?

https://codereview.appspot.com/71490045/

« Back to merge proposal