various: production code and logging improvements

various: production code and logging improvements

Fixed 4 slightly annoying, unrelated minor issues:
1. Refactored production code not to depend on
gocheck (manual provider and apiserver/charms).
As a drive-by fix, I refactored environs/manual
tests to be proper black-box unit tests, rather
than white-box tests in the manual package.
2. Fixed and improved tools download output via
curl and better handling of errors.
3. Added debug logging to environs sshstorage and
httpstorage, and the manual provider.
4. Reformatted and unified generated boilerplate
config across all providers, fixing bug #1221134 in
the process (-e env not properly explained).

Changes tested live with a manual and local environs
with added manually provisioned machines.

While testing I found out and filed this bug #1291292
(basically manual bootstrap is broken with ssl-hostname-
verification set to false, but this is due to a deeper
issue).

I tried to unify and fix when all commands report
"environment not bootstrapped", consistently across
providers, but it turned out like more work than I
originally thought, so I'll do a follow-up on that
(which will entail dropping support for 1.14 environments
without a .jenv file).

https://code.launchpad.net/~dimitern/juju-core/330-general-improvements/+merge/210108

(do not edit description out of merge proposal)

[dimitern, 2014-03-09 23:55:09]

Please take a look.

[axw, 2014-03-10 01:08:52]

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

https://codereview.appspot.com/72860045/diff/1/environs/manual/init.go#newcode27
environs/manual/init.go:27: cmd.Stdin =
strings.NewReader(ssh.CheckProvisionedScript)
utils/ssh is for *general ssh* things. These scripts do not belong there. I see
the case for exposing them, for the test code, but please keep them out of
utils/ssh.

[dimitern, 2014-03-10 20:32:46]

Please take a look.

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

https://codereview.appspot.com/72860045/diff/1/environs/manual/init.go#newcode27
environs/manual/init.go:27: cmd.Stdin =
strings.NewReader(ssh.CheckProvisionedScript)
On 2014/03/10 01:08:52, axw wrote:
> utils/ssh is for *general ssh* things. These scripts do not belong there. I
see
> the case for exposing them, for the test code, but please keep them out of
> utils/ssh.

Thanks for the explanation! As discussed on IRC, moved fakessh to
environs/manual/testing and the scripts in environs/manual.

[axw, 2014-03-11 04:43:21]

Thanks for the changes, and cleaning the code up. Looks much better now :)

Just a few little things...

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#ne...
environs/bootstrap/state.go:43: logger.Debugf("putting %q to boostrap storage
%#v", StateFile, storage)
s/boostrap/bootstrap/

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#n...
environs/configstore/disk.go:29: // Exists returns if the default disk-based
environment config storage
returns true iff?

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 {
Is this right? You're changing behaviour here; the old code only updated the
error if the existing error was non-nil.

[jameinel, 2014-03-11 10:06:32]

overall LGTM, though I did have some comments.

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 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.

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"
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.

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#newcode107
juju/api.go:107: }
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?

[rog, 2014-03-11 11:57:45]

review so far.

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#ne...
environs/bootstrap/state.go:43: logger.Debugf("putting %q to boostrap storage
%#v", StateFile, storage)
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.

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#n...
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?

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.

[rog, 2014-03-11 12:59:33]

A few more suggestions. It's great that we've lost the testing dependencies from
the code, but there are a few pieces I'm concerned about.

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
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.

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 {
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.

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.
> 

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.

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) {
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.

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)
This is changed only because of the unnecessary Exists patching AFAICS.

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 {
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).

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 {
Why do we have this?
Any client must call InitJujuHome. If they do not,
it is right to panic.

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#newc...
provider/joyent/config.go:23: # Can be set via env variables, or specified here
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>

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...
provider/local/environprovider.go:232: # Override the directory that is used for
the storage files and
# root-dir overrides the directory...

and below.

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#newc...
state/apiserver/charms.go:32: utilsstorage
"launchpad.net/juju-core/utils/storage"
Why not just "storage" ?

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...
state/apiserver/charms_test.go:27: utilsstorage
"launchpad.net/juju-core/utils/storage"
s/utilsstorage/storage/ ?

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

https://codereview.appspot.com/72860045/diff/2/state/apiserver/client/client_...
state/apiserver/client/client_test.go:1802: theStorage :=
s.Conn.Environ.Storage()
please not the dreaded "the" convention!

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

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

https://codereview.appspot.com/72860045/diff/2/utils/http.go#newcode41
utils/http.go:41: registerFileProto(transport)
Why are we doing this?

It seems like a potential security hole to me - there's nothing in the function
documentation to state that the transport also serves files from /.

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#newco...
utils/storage/storage.go:16: func GetEnvironStorage(st *state.State)
(storage.Storage, error) {
I don't think this package is justified for this trivial utility function.
Why not just put the function inside environs?

[dimitern, 2014-03-12 11:18:52]

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#ne...
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#ne...
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#n...
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#n...
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#newc...
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...
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#newc...
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...
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_...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/72860045/diff/2/state/apiserver/client/client_...
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#newco...
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().

[jameinel, 2014-03-12 12:13:00]

LGTM, though you should probably wait for Roger since he had the most comments
on it.

[rog, 2014-03-12 12:25:58]

On 2014/03/12 12:13:00, jameinel wrote:
> LGTM, though you should probably wait for Roger since he had the most comments
> on it.

LGTM, thanks!

[dimitern, 2014-03-12 12:34:30]

Please take a look.