LGTM
Worth landing with some tweaks, and iterating from here.


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

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config.go#newcode46
environs/openstack/config.go:46: func (c *environConfig) tenantId()
string {
On 2012/11/21 17:28:20, gz wrote:
> Using tenant-id vs. tenant-name is something we'll need to watch out
for later.

Just to follow up on this, "tenant-name" is what we'll get from the
environment, and what we would expect users to write themselves.
"tenant-id" would be something that we get from talking to the API.
So if we need to use it/store it we can, but ideally we would just go
with tenant name in the config.

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

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config_test.go#newcode18
environs/openstack/config_test.go:18: // Hook up gocheck into the gotest
runner.
On 2012/11/21 17:26:12, rog wrote:
> this is ubiquitous enough that it probably doesn't need a comment.

I would also mention that I don't usually see 1-line functions. They
pretty much are always wrapped to 3 lines. Am I mistaken about that?

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config_test.go#newcode110
environs/openstack/config_test.go:110: func (s *ConfigSuite) SetUpTest(c
*C) {
On 2012/11/21 17:28:20, gz wrote:
> This is kind of ugly, and we'll need to do better for testing envvars
as there
> are a bunch of different ones beyond these. The tests for this in the
python
> code used a change_environment helper that you passed a dict of new
values to
> within the test which were then stashed and restored at the end.

If we switch to using CredentialsFromEnv, do we need a lot of env
variable testing here? It seems like permutation testing can be done at
the goose level, and then some sort of smoke test can be done here.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config_test.go#newcode130
environs/openstack/config_test.go:130: {
If we are going to go with table based tests instead of a bunch of named
tests, I'd like us to add summary: to the definitions, so that we have
some sort of name identifying what this permutation is trying to do.
Table based tests aren't that different from using a TestSuite that has
a common SetUp and TearDown and then you get a named test case for each
one. Hopefully these have less infrastructure making them easier to use,
but they do have the downside of not having human-sensible names unless
we put them there.

As a thought, I wonder if we might try writing this as TestSuite with
some helper setup functions, rather than as one big 'check' function.
And see which one is clearer. I can see where table based tests have
nice definitions. But you also tend to end up with a 'check' function
that mixes setup and assertions.

The 'check' function can become overloaded with lots of if checks,
because some cases want to test a failure mode, some want to test
setting a value, etc.

So there are bits I like (simple table definition), and  bits I'm not
sure about (actual assertions are intermixed with a lot of setup code,
and many assertions are not actually valid for all of the rows in the
table.)

Is it worth trying?

https://codereview.appspot.com/6858052/