Please take a look.


https://codereview.appspot.com/72860045/diff/2/environs/bootstrap/state.go
File environs/bootstrap/state.go (right):

https://codereview.appspot.com/72860045/diff/2/environs/bootstrap/state.go#newcode43
environs/bootstrap/state.go:43: logger.Debugf("putting %q to boostrap
storage %#v", StateFile, storage)
On 2014/03/11 04:43:22, axw wrote:
> s/boostrap/bootstrap/

Done.

https://codereview.appspot.com/72860045/diff/2/environs/bootstrap/state.go#newcode43
environs/bootstrap/state.go:43: logger.Debugf("putting %q to boostrap
storage %#v", StateFile, storage)
On 2014/03/11 11:57:46, rog wrote:
> I'd be tempted lose this line entirely - it would be nicer to have
better
> debugging on the storage put methods. But if we keep it, I'd lose the
%#v - it
> could be very noisy. %T might be better.

Changed to %T as suggested.

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

https://codereview.appspot.com/72860045/diff/2/environs/configstore/disk.go#newcode29
environs/configstore/disk.go:29: // Exists returns if the default
disk-based environment config storage
On 2014/03/11 04:43:22, axw wrote:
> returns true iff?

I dropped this entirely.

https://codereview.appspot.com/72860045/diff/2/environs/configstore/disk.go#newcode29
environs/configstore/disk.go:29: // Exists returns if the default
disk-based environment config storage
On 2014/03/11 11:57:46, rog wrote:
> On 2014/03/11 04:43:22, axw wrote:
> > returns true iff?

> The canonical form is:

> // Exists reports whether ...

> I'm having difficulty understanding why this function exists though.
> For a start, the name "Exists" doesn't imply in which configstore
implementation
> the environment info exists.

> The only place that it's used in the production code, it seems that it
could be
> trivially replaced with a call to ReadInfo.

> It's easy to "mock" the existence of the environ info without
resorting to
> replacing this function - just create the info. That way our tests
don't become
> unnecessarily dependent on internal details of the code.

> I suggest removing this functon.

I dropped this entirely.

https://codereview.appspot.com/72860045/diff/2/environs/manual/init.go
File environs/manual/init.go (right):

https://codereview.appspot.com/72860045/diff/2/environs/manual/init.go#newcode21
environs/manual/init.go:21: const DetectionScript = `#!/bin/bash
On 2014/03/11 12:59:34, rog wrote:
> I don't quite see why this and CheckProvisionedScript now need to be
exported.

> The only reason that I can see is that manual/testing is a separate
package.
> Given that it's only imported from environs/manual, I don't see what
value that
> adds. I'd prefer to avoid unnecessary twisty dependencies.


DetectionScript and CheckProvisioned scripts need to be exported,
because they are used by both production code and tests.

https://codereview.appspot.com/72860045/diff/2/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/72860045/diff/2/environs/open.go#newcode93
environs/open.go:93: if source != ConfigFromInfo {
On 2014/03/11 10:06:33, jameinel wrote:
> On 2014/03/11 04:43:22, axw wrote:
> > Is this right? You're changing behaviour here; the old code only
updated the
> > error if the existing error was non-nil.

> So I think this means that if an environ was bootstrapped with 1.14
then you
> wouldn't have the .jenv. However, we don't need to preserve compat
with 1.14 if
> it makes things cleaner.
> 1.16+ should have always created a .jenv when it bootstrapped.


Exactly my point - 1.14 should be deprecated and the .jenv (if there)
should be sufficient to determine if the environment is bootstrapped or
not. I'll also link and close
https://bugs.launchpad.net/juju-core/+bug/1235217, which is no longer
relevant, and it will simplify the NewEnvFromName code.

https://codereview.appspot.com/72860045/diff/2/environs/open.go#newcode93
environs/open.go:93: if source != ConfigFromInfo {
On 2014/03/11 04:43:22, axw wrote:
> Is this right? You're changing behaviour here; the old code only
updated the
> error if the existing error was non-nil.

The error needs to be passed through in case it's "environment 'blah'
does not exists". If it's nil, we're fine, and the if block actually
accepts bootstrapped environments with a .jenv file.

But I decided this change won't be complete without its own dedicated
CL, which will make further simplifications in the code. So I'm dropping
this and reverting to the old behavior.

https://codereview.appspot.com/72860045/diff/2/environs/open.go#newcode93
environs/open.go:93: if source != ConfigFromInfo {
On 2014/03/11 12:59:34, rog wrote:
> I think andrew has a point here - if ConfigForName returns a useful
error
> message, it will be lost.

> If we're going to deprecate reading config from environments.yaml,
> that deserves its own CL, as there is a substantial amount
> of cleanup that can and should happen then.

Agreed - I'll do a follow-up.

https://codereview.appspot.com/72860045/diff/2/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/72860045/diff/2/errors/errors.go#newcode6
errors/errors.go:6: import "fmt"
On 2014/03/11 10:06:33, jameinel wrote:
> I actually prefer the other form.
> Because as soon as we need 2 imports, we want to do it anyway, and
that avoids
> having the noise of what actually changed confused with the extra
lines.

> Certainly not worth going around again, just wanted to give my feeling
that
> multiline imports are still reasonable to use when you only have one.


That's a consequence of using goimports, will fix it.

https://codereview.appspot.com/72860045/diff/2/errors/errors.go#newcode6
errors/errors.go:6: import "fmt"
On 2014/03/11 12:59:34, rog wrote:
> On 2014/03/11 10:06:33, jameinel wrote:
> > I actually prefer the other form.
> > Because as soon as we need 2 imports, we want to do it anyway, and
that avoids
> > having the noise of what actually changed confused with the extra
lines.
> >
> > Certainly not worth going around again, just wanted to give my
feeling that
> > multiline imports are still reasonable to use when you only have
one.
> >

> I think we shouldn't care too much. If goimports inserts a single
import, it
> does so without the ( ), and that's a useful enough tool that I think
it's
> reasonable to allow its default output through.

It does insert a single line, but if you edit it back to how it was it
doesn't change it, so as a tool it's reasonable :)

https://codereview.appspot.com/72860045/diff/2/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/72860045/diff/2/juju/api.go#newcode104
juju/api.go:104: if !configstore.Exists(envName) {
On 2014/03/11 12:59:34, rog wrote:
> As I mentioned in earlier comments, I don't think this is right.

> If we want to make it so that we can't open the API without a .jenv
file, that
> should be a deeper change in this file - there is a fair amount of
logic that
> can be simplified here.

Agreed - dropping this.

https://codereview.appspot.com/72860045/diff/2/juju/api.go#newcode107
juju/api.go:107: }
On 2014/03/11 10:06:33, jameinel wrote:
> I didn't see a change in apiconn_test.go that would actually exercise
this. Can
> you add one so we don't break it by accident?

I reverted the logic back how it was, so I'll focus on this + more tests
in a follow-up.

https://codereview.appspot.com/72860045/diff/2/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/72860045/diff/2/juju/apiconn_test.go#newcode257
juju/apiconn_test.go:257: bootstrapEnv(c, &cs.CleanupSuite,
coretesting.SampleEnvName, store)
On 2014/03/11 12:59:34, rog wrote:
> This is changed only because of the unnecessary Exists patching
AFAICS.

Yeah, but Exists is now dropped.

https://codereview.appspot.com/72860045/diff/2/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/72860045/diff/2/juju/conn_test.go#newcode88
juju/conn_test.go:88: s.PatchValue(&configstore.Exists, func(name
string) bool {
On 2014/03/11 12:59:34, rog wrote:
> This seems wrong, as I've mentioned earlier.
> It is not at all clear how patching this value will
> affect any of the lines of code below - it's truly
> action-at-a-distance, and hinges crucially on the
> fact that the code calls Exists rather than using
> the ReadInfo method to infer non-existence of an environment
> info (which is a perfectly reasonable implementation).

Dropped.

https://codereview.appspot.com/72860045/diff/2/juju/osenv/home.go
File juju/osenv/home.go (right):

https://codereview.appspot.com/72860045/diff/2/juju/osenv/home.go#newcode35
juju/osenv/home.go:35: func HaveJujuHome() bool {
On 2014/03/11 12:59:34, rog wrote:
> Why do we have this?
> Any client must call InitJujuHome. If they do not,
> it is right to panic.

I was using it before, but no longer - removed.

https://codereview.appspot.com/72860045/diff/2/provider/joyent/config.go
File provider/joyent/config.go (right):

https://codereview.appspot.com/72860045/diff/2/provider/joyent/config.go#newcode23
provider/joyent/config.go:23: # Can be set via env variables, or
specified here
On 2014/03/11 12:59:34, rog wrote:
> We should be consistent in the juju init output. Both ec2 and azure
have the
> descriptions as full sentences. Something like this would seem better
to me (we
> should really mention the actual environment variables used too):

> # sdc-user holds the SDC user. It can also be specified in
> # the SDC_ACCOUNT environment variable.
> #
> # sdc-user: <username>

Done.

https://codereview.appspot.com/72860045/diff/2/provider/local/environprovider.go
File provider/local/environprovider.go (right):

https://codereview.appspot.com/72860045/diff/2/provider/local/environprovider.go#newcode232
provider/local/environprovider.go:232: # Override the directory that is
used for the storage files and
On 2014/03/11 12:59:34, rog wrote:
> # root-dir overrides the directory...

> and below.

Done.

https://codereview.appspot.com/72860045/diff/2/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/72860045/diff/2/state/apiserver/charms.go#newcode32
state/apiserver/charms.go:32: utilsstorage
"launchpad.net/juju-core/utils/storage"
On 2014/03/11 12:59:34, rog wrote:
> Why not just "storage" ?

This is now removed.

https://codereview.appspot.com/72860045/diff/2/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (right):

https://codereview.appspot.com/72860045/diff/2/state/apiserver/charms_test.go#newcode27
state/apiserver/charms_test.go:27: utilsstorage
"launchpad.net/juju-core/utils/storage"
On 2014/03/11 12:59:34, rog wrote:
> s/utilsstorage/storage/ ?

This is now removed.

https://codereview.appspot.com/72860045/diff/2/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/72860045/diff/2/state/apiserver/client/client_test.go#newcode1802
state/apiserver/client/client_test.go:1802: theStorage :=
s.Conn.Environ.Storage()
On 2014/03/11 12:59:34, rog wrote:
> please not the dreaded "the" convention!

> Given that you've aliased both imports, you can
> continue to use just "storage".

That's a remnant of previous iterations, will change to "storage".

https://codereview.appspot.com/72860045/diff/2/utils/storage/storage.go
File utils/storage/storage.go (right):

https://codereview.appspot.com/72860045/diff/2/utils/storage/storage.go#newcode16
utils/storage/storage.go:16: func GetEnvironStorage(st *state.State)
(storage.Storage, error) {
On 2014/03/11 12:59:34, rog wrote:
> I don't think this package is justified for this trivial utility
function.
> Why not just put the function inside environs?

Changed to environs.GetStorage().

https://codereview.appspot.com/72860045/