Code review comment for lp:~maas-maintainers/juju-core/maas-provider-skeleton

Preliminary comments on the Environ...

Any points not badged with [FIX] should be considered usability/maintainability issues of varying importance.

1) _ "launchpad.net/juju-core/environs/maas"

Consider adding an environs/all package, that registers all providers except the dummy.

2) envCfg.attrs = v.(map[string]interface{})

Hmm... the attrs are all stored in the Config() anyway. Why are we duplicating them?

3) s/ecfgMutext/ecfgMutex/

4) quiesceStateFile [FIX if not necessary]

Only necessary if maas storage is not consistent.

5) uploadTools() [FIX: not necessary]

(Bootstrap no longer accepts uploadTools)

6) for a := longAttempt.Start(); len(stateAddrs) == 0 && a.Next(); { [FIX as (4)]

7) var err error = fmt.Errorf("(no error)")

I think this would be much clearer if we just break on err == nil inside the loop

8) StateInfo()

can't this use instances()?

9) environs.HighestVersion in StartInstance [FIX unless not unique]

This is wrong, but it might still match other providers.

10) environ.StopInstances([]environs.Instance{&instance})

Shouldn't this be a release?