Code review comment for lp:~wallyworld/juju-core/provisioner-api-supported-containers

I really like where this is going, but I think we're missing a couple
tests.

We should have a test that shows when Permission denied is triggered.

And I think we want to show what happens after api.Set* from the API
point of view.

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/machine.go#newcode246
state/api/provisioner/machine.go:246: var results
params.AddSupportedContainersResults
I just noticed that a few other places William changed the api.Client
objects to use

DoSomething([]list) into
DoSomething(...list)

I'm not sure which is more consistent in this case. If you want to look
around at other Machine functions and see if a pattern shows up. I'm
reasonably happy either way.

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/provisioner_test.go
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/provisioner_test.go#newcode418
state/api/provisioner/provisioner_test.go:418: c.Assert(containers,
gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
I like that you assert a refreshed State machine object notices the new
containers.
But you aren't actually asserting the api object itself sees the change.

So probably add one more assertion.

https://codereview.appspot.com/25480047/diff/1/state/apiserver/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/25480047/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode770
state/apiserver/provisioner/provisioner_test.go:770: }
We're missing a couple tests here about the permission model of the new
code.

We should be trying to change values from an object that isn't allowed.

It may be that anything that can get to the Provisioner api can set
anything that it would like. But I would *think* that only the
Provisioner running on machine-0 could set the container types for
machine-0. (machine-1 shouldn't be able to see or set the container
types for another arbitrary machine.)

https://codereview.appspot.com/25480047/