Use a map to store machine environment in config.

Use a map to store machine environment in config.

The MachineConfig now uses a map to store the environment
for the machine agent.

https://code.launchpad.net/~thumper/juju-core/machine-agent-environment/+merge/177707

(do not edit description out of merge proposal)

[thumper, 2013-07-30 22:33:52]

Please take a look.

[wallyworld, 2013-07-31 03:32:27]

LGTM with the cloudinit/cloudinit fix

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit.go#newcode75
environs/cloudinit.go:75: mcfg.MachineEnvironment["JUJU_PROVIDER_TYPE"] =
cfg.Type()
JUJU_PROVIDER_TYPE should be a const somewhere since it's used in a few places

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (left):

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:403: if cfg.StateServer {
Shouldn't we look in the map to ensure the provider type is set?

[thumper, 2013-07-31 04:17:48]

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit.go#newcode75
environs/cloudinit.go:75: mcfg.MachineEnvironment["JUJU_PROVIDER_TYPE"] =
cfg.Type()
On 2013/07/31 03:32:27, wallyworld wrote:
> JUJU_PROVIDER_TYPE should be a const somewhere since it's used in a few places

Yes, I have been thinking about this for a while.  Might as well do it now.

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (left):

https://codereview.appspot.com/12005048/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:403: if cfg.StateServer {
On 2013/07/31 03:32:27, wallyworld wrote:
> Shouldn't we look in the map to ensure the provider type is set?

It doesn't make it invalid like the others.  So, in this case, no I don't think
so.

I felt a little uneasy making the change before that put it in there.

[thumper, 2013-07-31 05:14:16]

Please take a look.

[thumper, 2013-07-31 05:14:49]

On 2013/07/31 05:14:16, thumper wrote:
> Please take a look.

I started updating some of the call sites to use the new osenv constants, but
realised there were LOTS, so stopped.

[thumper, 2013-07-31 05:55:04]

Please take a look.

[jtv.canonical, 2013-07-31 06:26:34]

LGTM, except you have conflicts.  Silly ones, from the looks of it: mostly
imports, but also a case where a function gained or lost an error return.

I was expecting to see changes to more providers' tests.  Don't those need to
set the provider type in various places as well?

[dave_cheney.net, 2013-07-31 06:41:36]

LGTM.

[thumper, 2013-07-31 21:46:02]

On 2013/07/31 06:26:34, jtv.canonical wrote:
> LGTM, except you have conflicts.  Silly ones, from the looks of it: mostly
> imports, but also a case where a function gained or lost an error return.
> 
> I was expecting to see changes to more providers' tests.  Don't those need to
> set the provider type in various places as well?

It is handled by the generic environs function FinishMachineConfig.