Please take a look.


https://codereview.appspot.com/85040046/diff/20001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/85040046/diff/20001/cmd/juju/addmachine.go#newcode123
cmd/juju/addmachine.go:123: // message.
On 2014/04/18 09:15:52, fwereade wrote:
> This feels a little bit off -- if we're not incompatible, we should
surely send
> the info in placement form? I think we should do this tweaking *after*
getting a
> NotImplemented...

Done. Sorry, misunderstood a previous comment about maintaining use of
ContainerType/ParentId. I now think you were referring to machine
templates.

https://codereview.appspot.com/85040046/diff/20001/instance/placement_test.go
File instance/placement_test.go (right):

https://codereview.appspot.com/85040046/diff/20001/instance/placement_test.go#newcode16
instance/placement_test.go:16: var parsePlacementTests = []struct {
On 2014/04/18 09:15:52, fwereade wrote:
> FWIW, I really like it when test tables are defined inside the funcs
that use
> them, instead of cluttering up the package globals ;). It's a bit
surprising at
> first, but the structure is actually quite clean:

> for i, test := range []struct{
>      // fields
> }{{
>      // test
> }, {
>      // test
> }} {
>      // code
> }

Sure, I just copied the structure from instance_test.go.

Updated. I don't like munging the slice literal into the range, as it
puts the assigned variables too far away from the loop body.

https://codereview.appspot.com/85040046/diff/20001/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/85040046/diff/20001/provider/maas/environ.go#newcode173
provider/maas/environ.go:173: // TODO(axw) handle maas-name placement
directive
On 2014/04/18 09:15:52, fwereade wrote:
> deserves a bug#

Done.

https://codereview.appspot.com/85040046/diff/20001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/85040046/diff/20001/state/api/client.go#newcode230
state/api/client.go:230: // This exists for backwards compatibility;
remove in 1.21 (client only).
On 2014/04/18 09:15:52, fwereade wrote:
> are we sure we can drop this? I fear we'll have to deal with the
possibility of
> a 1.18 client *or* server pretty-much-forever

I'm talking about the client code only, and then only from 1.21 when we
no longer require new-client-to-1.18-server compat.

https://codereview.appspot.com/85040046/diff/20001/state/api/client.go#newcode246
state/api/client.go:246: err := c.call("AddMachinesV2", args, results)
On 2014/04/18 09:15:52, fwereade wrote:
> grump, rage. jam, *please* get me some API versioning.

I didn't want to do this either.

https://codereview.appspot.com/85040046/diff/20001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/85040046/diff/20001/state/api/params/params.go#newcode714
state/api/params/params.go:714: Results []PlacementResult
On 2014/04/18 09:15:52, fwereade wrote:
> This feels like it should be all together in a single call that
extracts all the
> info we need for a given machine -- ie we should be able to grab
> series+constraints+placement (and I guess instance-group) for N
machines in a
> single call.

> Maybe out of scope here, but I feel obliged to bang the
chatty-APIs-are-bad drum
> at every opportunity.

Done.

https://codereview.appspot.com/85040046/diff/20001/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/85040046/diff/20001/state/apiserver/client/client_test.go#newcode1702
state/apiserver/client/client_test.go:1702: apiParams[0].Placement =
instance.MustParsePlacement("lxc")
On 2014/04/18 09:15:52, fwereade wrote:
> A thought -- can we at least make what's happening explicit at the API
level?
> parse "lxc" to "lxc:new", or something?

> If this is stupid, disregard.

I'm not a fan, because if we want to use non-numeric directives for
containers in the future, then we have some arbitrary string ("new") to
work around. The empty string is always going to be a special case, and
I think makes sense in this context (empty string == give me an LXC
container on no specific machine).

https://codereview.appspot.com/85040046/diff/20001/worker/provisioner/provisioner_task.go
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/85040046/diff/20001/worker/provisioner/provisioner_task.go#newcode467
worker/provisioner/provisioner_task.go:467: machinePlacement, err :=
machine.Placement()
On 2014/04/18 09:15:52, fwereade wrote:
> I have a half-suspicion that we can actually safely tack Placement
onto some
> existing API call -- if we talk to an old state server we don't get
placement,
> but we wouldn't anyway (and hopefully we won't talk to old state
servers at all,
> anyway, soon enough. Crazy?

> It's really just that people tend to follow existing conventions,
whether or not
> they're good ones, and every chatty API call normalises the behaviour
in
> people's minds.

I have created a new API method, ProvisioningInfo, and removed the old
Constraints and RequestedNetworks ones. Series is still in use in one
other place.

https://codereview.appspot.com/85040046/