Better error when region is misconfigured.

Better error when region is misconfigured.

This branch prints a better error message if the user hasn't configured their
region correctly. For example, if they specify the swift region, but not the
fully qualified compute region, they are told that and the list of correct 
regions is displayed. Similarly, if the region is totally wrong, they are 
told that too.

The nexted error have also been reformatted to make them more readable, eg

error: failed to GET object provider-state from container juju-50548874395ac32d8896f65ab33f3e17
caused by: cannot create service URLs
caused by: invalid region "xegion-a.geo-1"
one of these regions may be suitable instead: "az-1.region-a.geo-1, az-2.region-a.geo-1, az-3.region-a.geo-1"

https://code.launchpad.net/~wallyworld/goose/better-misconfig-error/+merge/155889

(do not edit description out of merge proposal)

[wallyworld, 2013-03-28 05:09:31]

Please take a look.

[jameinel, 2013-03-28 06:39:43]

LGTM

This is the new error message:

$ ./juju bootstrap -e hp-jam
error: cannot query old bootstrap state: failed to GET object provider-state
from container juju-jamtest-dc8d41bee05c605f, caused by: cannot create service
URLs, caused by: the configured region "region-a.geo-1" does not allow access to
all required services, namely: "compute, object-store", access to these services
is missing: "compute", one of these regions may be suitable instead:
"az-1.region-a.geo-1, az-2.region-a.geo-1, az-3.region-a.geo-1"


I don't think this is perfect, but it does contain a nice amount of context. My
only complaint is that the formatting makes it just look like a big blob of text
that users may not ever read.

Would it be reasonable to put some '\n' in there?

[dimitern, 2013-03-28 08:23:39]

Somewhat tentative LGTM, although I'd suggest a bit of refactoring the deeply
nested for loops in that function.
Otherwise, I think this is a very good change to have!

https://codereview.appspot.com/8067044/diff/1/client/client.go
File client/client.go (right):

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode73
client/client.go:73: // The service types which must be available after
authentication,
put a line break between this and previous lines please.

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode211
client/client.go:211: errorPrefix = fmt.Sprintf("the configured region %q does
not allow access to all required services, namely: %q, "+
%q -> %s ("x", "y") - see below.

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode212
client/client.go:212: "access to these services is missing: %q",
ditto

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode219
client/client.go:219: return fmt.Errorf("%s, one of these regions may be
suitable instead: %q",
instead of %q here, it's better to print out %s, which has each region name
enclosed in quotes, like:
"region1", "region2", "region3"
instead:
"region1, region2, region3"
otherwise the user might try to use the comma-delimited list of regions as
region name in the config.

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode230
client/client.go:230: // possibleRegions returns a list of regions, any of which
will allow the client to access the required service types.
please use some line breaks to separate logical parts of the code here, it's a
bit hard to read like this.

 in addition, I think it's better to extract some bits of the deeply nested
loops as helpers outside, to reduce the nestiness depth.

https://codereview.appspot.com/8067044/diff/1/client/local_test.go
File client/local_test.go (right):

https://codereview.appspot.com/8067044/diff/1/client/local_test.go#newcode284
client/local_test.go:284: var missingEndpointMsgf = ".*the configured region %q
does not allow access to all required services, namely: %q, access to these
services is missing: %q"
same points on %q vs. %s ("x", "y") in messages

https://codereview.appspot.com/8067044/diff/1/client/local_test.go#newcode321
client/local_test.go:321: c.Assert(err.Error(), Matches, at.errorMsg)
why not c.Assert(err, ErrorMatches, at.errorMsg) ?

[wallyworld, 2013-03-28 09:07:55]

Please take a look.

[jameinel, 2013-03-28 09:44:27]

LGTM

https://codereview.appspot.com/8067044/diff/8001/client/client.go
File client/client.go (right):

https://codereview.appspot.com/8067044/diff/8001/client/client.go#newcode212
client/client.go:212: "access to these services is missing: %q",
This is one of the few places where you have "\n " rather than "\nstart of next
line".

Did you mean to indent the ' access'?

[wallyworld, 2013-03-28 09:50:34]

https://codereview.appspot.com/8067044/diff/8001/client/client.go
File client/client.go (right):

https://codereview.appspot.com/8067044/diff/8001/client/client.go#newcode212
client/client.go:212: "access to these services is missing: %q",
On 2013/03/28 09:44:28, jameinel wrote:
> This is one of the few places where you have "\n " rather than "\nstart of
next
> line".
> 
> Did you mean to indent the ' access'?

Done.

[dimitern, 2013-03-28 10:22:24]

So you basically ignored all my suggestions and didn't even reply..

[wallyworld, 2013-03-28 12:39:25]

Sorry, I totally missed doing these suggestions. I've made the fixes in branch
lp:~wallyworld/goose/region-error-fixups

https://codereview.appspot.com/8067044/diff/1/client/client.go
File client/client.go (right):

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode73
client/client.go:73: // The service types which must be available after
authentication,
On 2013/03/28 08:23:39, dimitern wrote:
> put a line break between this and previous lines please.

Done.

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode211
client/client.go:211: errorPrefix = fmt.Sprintf("the configured region %q does
not allow access to all required services, namely: %q, "+
On 2013/03/28 08:23:39, dimitern wrote:
> %q -> %s ("x", "y") - see below.

Done.

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode212
client/client.go:212: "access to these services is missing: %q",
On 2013/03/28 08:23:39, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode219
client/client.go:219: return fmt.Errorf("%s, one of these regions may be
suitable instead: %q",
On 2013/03/28 08:23:39, dimitern wrote:
> instead of %q here, it's better to print out %s, which has each region name
> enclosed in quotes, like:
> "region1", "region2", "region3"
> instead:
> "region1, region2, region3"
> otherwise the user might try to use the comma-delimited list of regions as
> region name in the config.

Done.

https://codereview.appspot.com/8067044/diff/1/client/client.go#newcode230
client/client.go:230: // possibleRegions returns a list of regions, any of which
will allow the client to access the required service types.
On 2013/03/28 08:23:39, dimitern wrote:
> please use some line breaks to separate logical parts of the code here, it's a
> bit hard to read like this.
> 
>  in addition, I think it's better to extract some bits of the deeply nested
> loops as helpers outside, to reduce the nestiness depth.

Done.

https://codereview.appspot.com/8067044/diff/1/client/local_test.go
File client/local_test.go (right):

https://codereview.appspot.com/8067044/diff/1/client/local_test.go#newcode284
client/local_test.go:284: var missingEndpointMsgf = ".*the configured region %q
does not allow access to all required services, namely: %q, access to these
services is missing: %q"
On 2013/03/28 08:23:39, dimitern wrote:
> same points on %q vs. %s ("x", "y") in messages

Done.

https://codereview.appspot.com/8067044/diff/1/client/local_test.go#newcode321
client/local_test.go:321: c.Assert(err.Error(), Matches, at.errorMsg)
On 2013/03/28 08:23:39, dimitern wrote:
> why not c.Assert(err, ErrorMatches, at.errorMsg) ?

Done.