Implemented the base of a juju openstack provider.

Implemented the base of a juju openstack provider.

Right now the configuration is parsed and validated, and the minimum amount of code needed in the provider is implemented, the rest is stubbed out for now.

https://code.launchpad.net/~dimitern/juju-core/openstack-stub-provider-config/+merge/135454

(do not edit description out of merge proposal)

[dimitern, 2012-11-21 17:02:06]

Please take a look.

[rog, 2012-11-21 17:26:11]

looks great, with only minor comments below.

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#ne...
environs/openstack/config.go:79: ecfg.username() != "" ||
perhaps these checks should be in dummyEnvAuth, and you could just return err
here. then the error message can be more useful.

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....
environs/openstack/config_test.go:18: // Hook up gocheck into the gotest runner.
this is ubiquitous enough that it probably doesn't need a comment.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config_test....
environs/openstack/config_test.go:111: s.savedUsername =
os.Getenv("OS_USERNAME")
there are perhaps enough of these that you could have a variable above, say
envVars:

var envVars = map[string]string{
    "OS_USERNAME" : "testuser",
    etc
}

then have ConfigSuite.savedVars defined as map[string]string

and below:

for v, val := range envVars {
   s.savedVargs[v] = os.Getenv(v)
   os.Setenv(v, val)
}

and restore in TearDownTest.

YMMV, just a thought.

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

https://codereview.appspot.com/6858052/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:21: //
-------------------------------------------------------------------
d

we don't tend to bother with markers like this.
i'm not sure the comment below helps much either. it should  be fairly obvious
that this is implementing environs.Environ.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:66: // TODO: setup the goose client auth/compute,
etc. here
TODO(dimitern)

(we usually attach user names to TODOs)

https://codereview.appspot.com/6858052/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:119: // EnvironProvider interface
as for Environ comment above.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:121: func (p environProvider) Open(cfg
*config.Config) (environs.Environ, error) {
i think it's worth putting these methods at the top of the file, as in ec2 - an
EnvironProvider is a precursor to the Environ, and it has many less methods and
much less code.

[gz, 2012-11-21 17:28:19]

Have made a selection of comments on specifics, some of which is just for things
to worry about in future.

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#ne...
environs/openstack/config.go:16: "identity-url": schema.String(),
Where possible, use the existing keys names that python juju does, so 'auth-url'
rather than 'identity-url' here.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:46: func (c *environConfig) tenantId() string {
Using tenant-id vs. tenant-name is something we'll need to watch out for later.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:50: func (c *environConfig) identityURL() string {
As above, 'auth-url' instead.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:66: func (p environProvider) Validate(cfg, old
*config.Config) (valid *config.Config, err error) {
Having one big function in the style of the current go code will get painful
with the various different authentication schemes openstack uses, but we will
want to delegate as much as possible to goose.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:115: // TODO: temporarily here, until goose client
handles this
You should be able to use Credentials from the identity package, the client has
now been refactored to look at that.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:120: func dummyEnvAuth() (dummyAuth, error) {
Just use CredentialsFromEnv from the goose identity package as well.

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....
environs/openstack/config_test.go:40: func (t configTest) check(c *C) {
This function is already big and scary and will presumably only get worse when
we add more authentication variations. Is there not a better way?

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config_test....
environs/openstack/config_test.go:110: func (s *ConfigSuite) SetUpTest(c *C) {
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.

https://codereview.appspot.com/6858052/diff/1/environs/openstack/config_test....
environs/openstack/config_test.go:132: "region": "lcy01",
I'd prefer not to bundle real canonistack values in here.

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

https://codereview.appspot.com/6858052/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:137: m["username"] = ecfg.username()
What was SecretAttrs for exactly? Potentially the tenant need not/should not be
secret.

[gz, 2012-11-21 17:59:15]

Remembered that jam has already written a neat way of handling envvar isolation
in goose, that may want pulling out somewhere common so it can be used here too.

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....
environs/openstack/config_test.go:110: func (s *ConfigSuite) SetUpTest(c *C) {
See also what goose/testing/envsuite provides, which is a neat alternative.

[jameinel, 2012-11-22 07:19:48]

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

[dimitern, 2012-11-22 10:46:47]

Please take a look.

[gz, 2012-11-22 12:04:58]

Changes all look good to me.

[niemeyer, 2012-11-22 12:54:14]

Nice. LGTM with trivials only:

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

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:54: func (c *environConfig) container() string {
As we discussed, let's use controlBucket/"control-bucket" for this setting, for
compatibility with the ec2 provider.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:75: ecfg.authURL() == "" {
Can we please have these in the same line? We have lines around the same width
around here, and it ends up more reasonable as it doesn't align with the
internal block.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:79: return nil, fmt.Errorf("environment has no
username, password, tenant-name, or auth-url")
"OpenStack environment requires options username, password, tenant-name, and
auth-url"

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:112: type dummyAuth struct {
Why dummy? This looks quite real. I think it should be envAuth or something like
that.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:116: func dummyEnvAuth() (dummyAuth, error) {
getEnvAuth?

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:126: auth.authURL == "" {
Same line as well, please.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:127: return auth, fmt.Errorf("missing username,
password, tenant-name, or auth-url")
This function can be simplified to return (auth envAuth, ok bool) instead. This
error is pretty much the same one found above, and is being discarded, which
means the call site above is inferring what error it is.

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

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config_te...
environs/openstack/config_test.go:133: var configTests = []configTest{
Nice tests.

[dimitern, 2012-11-22 13:13:16]

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

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:54: func (c *environConfig) container() string {
On 2012/11/22 12:54:14, niemeyer wrote:
> As we discussed, let's use controlBucket/"control-bucket" for this setting,
for
> compatibility with the ec2 provider.

Done.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:75: ecfg.authURL() == "" {
On 2012/11/22 12:54:14, niemeyer wrote:
> Can we please have these in the same line? We have lines around the same width
> around here, and it ends up more reasonable as it doesn't align with the
> internal block.

Done.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:79: return nil, fmt.Errorf("environment has no
username, password, tenant-name, or auth-url")
On 2012/11/22 12:54:14, niemeyer wrote:
> "OpenStack environment requires options username, password, tenant-name, and
> auth-url"

Done.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:112: type dummyAuth struct {
On 2012/11/22 12:54:14, niemeyer wrote:
> Why dummy? This looks quite real. I think it should be envAuth or something
like
> that.
This is here only because the goose identity package will handle this
eventually, but for now it's like this (hence the TODO), so we don't couple the
unstable code here. Anyway, renamed as suggested.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:116: func dummyEnvAuth() (dummyAuth, error) {
On 2012/11/22 12:54:14, niemeyer wrote:
> getEnvAuth?

Done.

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go...
environs/openstack/config.go:126: auth.authURL == "" {
On 2012/11/22 12:54:14, niemeyer wrote:
> Same line as well, please.

Done.

[dimitern, 2012-11-22 13:13:42]

Please take a look.

[fwereade, 2012-11-22 14:22:10]

LGTM

[dimitern, 2012-11-22 15:02:36]

On 2012/11/22 14:22:10, fwereade wrote:
> LGTM

As discussed, added checks for empty region and invalid auth-url.

[dimitern, 2012-11-22 15:03:10]

Please take a look.

[rog, 2012-11-22 15:04:22]

On 2012/11/22 15:03:10, dimitern wrote:
> Please take a look.

LGTM

[dimitern, 2012-11-22 15:05:57]

*** Submitted:

Implemented the base of a juju openstack provider.

Right now the configuration is parsed and validated, and the minimum amount of
code needed in the provider is implemented, the rest is stubbed out for now.

R=rog, gz, john.meinel, niemeyer, fwereade
CC=
https://codereview.appspot.com/6858052