AddSupportedContainers on provisioner machine api

AddSupportedContainers on provisioner machine api

The AddSupportedContainers api on the provisioner.Machine
will be used by future changes to machine agent initialisation.

A change was also made to the server side implementation so that
the machine doc txn-revno is no longer checked. It was causing issues
with multi-threaded tests where different fields in the machine doc
were being updated. The machine agent initialisation thread is the
only one that updates the supported containers set.

https://code.launchpad.net/~wallyworld/juju-core/provisioner-api-supported-containers/+merge/194982

(do not edit description out of merge proposal)

[wallyworld, 2013-11-13 04:24:16]

Please take a look.

[jameinel, 2013-11-13 09:27:52]

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....
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/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/provisio...
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/pr...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/25480047/diff/1/state/apiserver/provisioner/pr...
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.)

[wallyworld, 2013-11-14 00:58:07]

Please take a look.

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....
state/api/provisioner/machine.go:246: var results
params.AddSupportedContainersResults
On 2013/11/13 09:27:52, jameinel wrote:
> 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.

Several other api methods use ... so I changed to that.

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

https://codereview.appspot.com/25480047/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:418: c.Assert(containers,
gc.DeepEquals, []instance.ContainerType{instance.LXC, instance.KVM})
On 2013/11/13 09:27:52, jameinel wrote:
> 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.

The API doesn't provider a getter for supported containers since it doesn't need
it (the API is used by the machine agent to set the supported containers). So
nothing to test.

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

https://codereview.appspot.com/25480047/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:770: }
On 2013/11/13 09:27:52, jameinel wrote:
> 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.)

Done.

[jameinel, 2013-11-14 10:38:00]

LGTM

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

https://codereview.appspot.com/25480047/diff/20001/state/apiserver/provisione...
state/apiserver/provisioner/provisioner_test.go:801: })
The only other test I've seen here is a machine that doesn't exist still shows
up as ErrUnauthorized rather than ErrNotFound.
I think you can do that with just adding 

{
   MachineTag: "machine-42",
   ...
},

[wallyworld, 2013-11-14 23:59:11]

On 2013/11/14 10:38:00, jameinel wrote:
> LGTM
> 
>
https://codereview.appspot.com/25480047/diff/20001/state/apiserver/provisione...
> File state/apiserver/provisioner/provisioner_test.go (right):
> 
>
https://codereview.appspot.com/25480047/diff/20001/state/apiserver/provisione...
> state/apiserver/provisioner/provisioner_test.go:801: })
> The only other test I've seen here is a machine that doesn't exist still shows
> up as ErrUnauthorized rather than ErrNotFound.
> I think you can do that with just adding 
> 
> {
>    MachineTag: "machine-42",
>    ...
> },

Done