Implement Environ.SetConfig for Azure provider.

Implement Environ.SetConfig for Azure provider.



https://code.launchpad.net/~jtv/juju-core/setconfig/+merge/170646

(do not edit description out of merge proposal)

[jtv.canonical, 2013-06-20 15:24:50]

Please take a look.

[wallyworld, 2013-06-21 00:50:00]

LGTM

[fwereade, 2013-06-21 10:26:53]

not lgtm without config-change validation, but it should be a pretty trivial
change.

https://codereview.appspot.com/10407045/diff/1/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/10407045/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:81: 
I think we need a ValidateConfig check to ensure old and new match (if old
exists).

https://codereview.appspot.com/10407045/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/10407045/diff/1/environs/azure/environ_test.go...
environs/azure/environ_test.go:157: err = env.SetConfig(cfg)
This should fail, because you're setting an incompatible config. It will do so
if you add the validation suggested in the other file.

[jtv.canonical, 2013-06-21 15:27:14]

On 2013/06/21 10:26:53, fwereade wrote:
> not lgtm without config-change validation, but it should be a pretty trivial
> change.

>
https://codereview.appspot.com/10407045/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:81: 
> I think we need a ValidateConfig check to ensure old and new match (if old
> exists).

Thanks for noticing that.  Here's the weird thing.  Of all the other providers,
AFAICS only the MAAS one does that — and it was largely cargo-culted from the
others.

The Validate call looks entirely generic (as does the top of Validate itself) so
that it doesn't seem to belong in the providers.  Wasn't all this going to be
made the caller's responsibility, as in SetEnvironmentCommand.Run?

I don't see a ValidateConfig in the codebase, but I presume you mean the
provider's Validate (which in turn calls config.Validate).

[jtv.canonical, 2013-06-24 10:27:14]

Please take a look.

[fwereade, 2013-06-24 12:30:19]

LGTM with explicit testing of config errors. Users might see them ;).

https://codereview.appspot.com/10407045/diff/7001/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/10407045/diff/7001/environs/azure/environ_test...
environs/azure/environ_test.go:104: c.Check(err, NotNil)
Please check the actual error -- we've found that NotNils (and overly loose
ErrorMatches tests) tend to sweep under the rug the ugliness of some of our
error messages. At the moment, at least, we'd rather see tests fail because of
distant error changes, so we have a chance to notice and resolve such problems
before users see them.

https://codereview.appspot.com/10407045/diff/7001/environs/azure/environ_test...
environs/azure/environ_test.go:157: c.Assert(err, NotNil)
ditto.

[jtv.canonical, 2013-06-24 15:21:01]

Thanks.  Changes made.