Code review comment for lp:~dimitern/juju-core/openstack-stub-provider-config

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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#newcode54
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#newcode75
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#newcode79
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#newcode112
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#newcode116
environs/openstack/config.go:116: func dummyEnvAuth() (dummyAuth, error)
{
getEnvAuth?

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

https://codereview.appspot.com/6858052/diff/5002/environs/openstack/config.go#newcode127
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_test.go
File environs/openstack/config_test.go (right):

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

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

« Back to merge proposal