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

11) "maas-oauth": schema.String(),

This should probably be a custom schema.Checker

12) validCfg, err := prov.Validate(cfg, nil)

thumper's just landing some changes around this area: there's another method you need to call in Validate, IIRC, that checks old/new configs match sanely (no changing env names, for example).

13) "ca-private-key": testing.CAKey,

I'd have authorized-keys and admin-secret in here rather than duplicated in every test. While you're at it, please add `"agent-version": "1.2.3",` or something (it'll fit better with an upcoming branch of mine).

14) inst, err := env.startBootstrapNode(tools,

An upcoming branch will change this almost beyond recognition, but only for the better. BTW, is constraints support expected imminently? If not, please log.Warningf that they're ignored.

15) env.ecfgUnlocked = ecfg

There should be some validation that the two configs match, per thumper's branch. If we forgot it elsewhere, which we might have done, don't consider this a blocker.

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

upgrading (10) to a FIX

17) flags := environs.HighestVersion

crossgrading (9) to a changing-beyond-recognition

18) _, err := blah();\n if err != nil {

several places:

if _, err := blah(); err != nil {

19) client := environ.maasClientUnlocked.GetSubObject("nodes/") [FIX]

The intended model is that Environ methods be safe from any goroutine. Use a maasClient() helper throughout to get a client safely. (see `// To properly observe e.storageUnlocked `)

It may very well be the case that there's actually no point making Environs goroutine-safe, but I'm not going to analyse that now.

20) errSetConfig := env.SetConfig(cfg2)

Changing names is actively bad behaviour, please stick to sensible changes like default-series and authorized-keys :).

21) package maas

I don't really like having all these tests internal -- most of them should really go into maas_test, I think

22) func fakeWriteCertAndKey(

unnecessary?

23) === added file 'environs/maas/environprovider.go'

(and similar small files with clear purposes) yay! I like this, we should do it more ;)

24) func createTempFile(c *C

create it inside a c.MkDir() and it'll be cleaned up for you

25) info.serializeYAML()

Please use goyaml.Marshal and export/tag the struct fields where necessary

~~~~~~~~~~~~~~

Thanks, and sorry for the delay. This is looking really solid so far; if it's actually spinning up environments and deploying units now, then I'd like to get it in as soon as possible and starting working on rationalizing the testing then. I have no problem with the coverage, just of where the tests are (and that none of the jujutest tests have been hooked).