Fix OpenStack config issues

Fix OpenStack config issues

This branch removes the hacks put in place to fix some OpenStack tests, and reworks the config processing so that config values are loaded properly from the YAML environment.

https://code.launchpad.net/~wallyworld/juju-core/fix-openstack-test-hack/+merge/139364

(do not edit description out of merge proposal)

[wallyworld, 2012-12-12 01:39:06]

Please take a look.

[fwereade, 2012-12-17 07:55:12]

Code looks solid, but I'd prefer a bit more testing.

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config_test.go
File environs/openstack/config_test.go (left):

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config_test....
environs/openstack/config_test.go:290: }
Would you please add a few quick tests to verify that the config does behave
correctly when it gets creds from the environment?

[dimitern, 2012-12-17 10:28:58]

Just a comment

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:81: missingAttributef := "required environment
variable not set for credentials attribute: %s"
should be "OpenStack environment has no region" (think of the end-user seeing
this). Also for the other cases. Possibly change ifs to a switch { ... }

[wallyworld, 2012-12-18 01:13:04]

Please take a look.

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:81: missingAttributef := "required environment
variable not set for credentials attribute: %s"
On 2012/12/17 10:28:58, dimitern wrote:
> should be "OpenStack environment has no region" (think of the end-user seeing
> this). Also for the other cases. Possibly change ifs to a switch { ... }

The text of the error message comes from Goose and is merely referenced here.
I'll see if I can coordinate Goose changes with this branch, as they both will
need to land together.

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config_test.go
File environs/openstack/config_test.go (left):

https://codereview.appspot.com/6923056/diff/1/environs/openstack/config_test....
environs/openstack/config_test.go:290: }
On 2012/12/17 07:55:12, fwereade wrote:
> Would you please add a few quick tests to verify that the config does behave
> correctly when it gets creds from the environment?

Done.

[jameinel, 2012-12-18 13:38:23]

LGTM, though I'm a bit concerned if the tests are leaving global state (env
variables) mutated.

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
environs/openstack/config_test.go:260: }
This feels like something that should be in SetUpTest. It also is something that
you aren't cleaning up the env when the test tears down (that I can tell). So
doesn't this leave global state mutated after each test runs? (So other tests
may accidentally pass/fail).

I realize the map might already be tearing everything down. So if I'm wrong,
nothing specifically needs to be done here.

[fwereade, 2012-12-18 14:25:31]

config looks good; provider looks good (*assuming* the lack of testing is
justified by a similar lack in ec2... is that really the case?); but the local
test is not sane.

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
environs/openstack/config_test.go:260: }
On 2012/12/18 13:38:23, jameinel wrote:
> This feels like something that should be in SetUpTest. It also is something
that
> you aren't cleaning up the env when the test tears down (that I can tell). So
> doesn't this leave global state mutated after each test runs? (So other tests
> may accidentally pass/fail).
> 
> I realize the map might already be tearing everything down. So if I'm wrong,
> nothing specifically needs to be done here.

SetUpTest/TearDownTest are doing what they should, so I think this is just fine.

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
environs/openstack/config_test.go:266: test := configTests[11]
Please take configTests[11] (and for that matter configTests[0]) and define them
outside the [], with actual names, so we can refer to them sanely :).

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
environs/openstack/local_test.go:29: openstack.UseTestMetadata(true)
This is the only line actually needed to set up this test. Please drop the
misleading excess junk :).

(This sort of stuff will, no doubt, go into a new test case when we have some
tests that expect an environment -- for now, all we're testing is things that
*must* work when there is *no* environment, and so creating an environment is
just plain wrong.)

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
environs/openstack/local_test.go:40: p := s.env.Provider()
p = environs.Provider("openstack")

(I think, something like that anyway -- regardless, we must not require an env)

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
environs/openstack/local_test.go:47: p := s.env.Provider()
Likewise

[wallyworld, 2012-12-19 04:09:11]

On 2012/12/18 14:25:31, fwereade wrote:
> config looks good; provider looks good (*assuming* the lack of testing is
> justified by a similar lack in ec2... is that really the case?); 

There are some unit type tests for the ec2 provider but there's a TODO saying
they need to be moved to jujutest.LiveTests so once that is done, all providers
can benefit. Hence I didn't want to spend too much wasted time here writing
essentially duplicate tests. The provider is tested at the moment by running the
live juju integration tests.

but the local
> test is not sane.
> 
>
https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
> File environs/openstack/config_test.go (right):
> 
>
https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
> environs/openstack/config_test.go:260: }
> On 2012/12/18 13:38:23, jameinel wrote:
> > This feels like something that should be in SetUpTest. It also is something
> that
> > you aren't cleaning up the env when the test tears down (that I can tell).
So
> > doesn't this leave global state mutated after each test runs? (So other
tests
> > may accidentally pass/fail).
> > 
> > I realize the map might already be tearing everything down. So if I'm wrong,
> > nothing specifically needs to be done here.
> 
> SetUpTest/TearDownTest are doing what they should, so I think this is just
fine.
> 
>
https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
> environs/openstack/config_test.go:266: test := configTests[11]
> Please take configTests[11] (and for that matter configTests[0]) and define
them
> outside the [], with actual names, so we can refer to them sanely :).
> 
>
https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
> File environs/openstack/local_test.go (right):
> 
>
https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
> environs/openstack/local_test.go:29: openstack.UseTestMetadata(true)
> This is the only line actually needed to set up this test. Please drop the
> misleading excess junk :).
> 
> (This sort of stuff will, no doubt, go into a new test case when we have some
> tests that expect an environment -- for now, all we're testing is things that
> *must* work when there is *no* environment, and so creating an environment is
> just plain wrong.)
> 
>
https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
> environs/openstack/local_test.go:40: p := s.env.Provider()
> p = environs.Provider("openstack")
> 
> (I think, something like that anyway -- regardless, we must not require an
env)
> 
>
https://codereview.appspot.com/6923056/diff/6001/environs/openstack/local_tes...
> environs/openstack/local_test.go:47: p := s.env.Provider()
> Likewise

[wallyworld, 2012-12-19 06:26:42]

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/6923056/diff/6001/environs/openstack/config_te...
environs/openstack/config_test.go:266: test := configTests[11]
On 2012/12/18 14:25:31, fwereade wrote:
> Please take configTests[11] (and for that matter configTests[0]) and define
them
> outside the [], with actual names, so we can refer to them sanely :).

I copied one how one of the ec2 tests did it. But yes, it should be fixed.

[wallyworld, 2012-12-19 06:59:43]

*** Submitted:

Fix OpenStack config issues

This branch removes the hacks put in place to fix some OpenStack tests, and
reworks the config processing so that config values are loaded properly from the
YAML environment.

R=fwereade, dimitern, jameinel
CC=
https://codereview.appspot.com/6923056