Code review comment for lp://staging/~fwereade/pyjuju/go-place-unit

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Implementation looks nice. I have a couple of API cleanup suggestions
for you to consider. Please let me know what you think.

https://codereview.appspot.com/6248076/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode26
state/unit.go:26: type PlacementPolicy string
Can we name this as AssignmentPolicy? I'd also be equally happy to go to
the other direction and name methods as PlaceInMachine etc.

https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode30
state/unit.go:30: PlaceUnassigned PlacementPolicy = "unassigned"
It'd be nice to have a sentence on top of each explaining their meaning.

https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode241
state/unit.go:241: func (u *Unit) Place(policy PlacementPolicy) (err
error) {
It's awkward to have this as a method of the unit. Not only is it using
the unit just through its API, but it's also digging through and
*creating machines* silently.

IMO, this is better suited for a utility function such as:

func AssignUnit(st *State, u *Unit, policy AssignmentPolicy)

(or Place/Placement, depending on which side you prefer)

It'd be useful as well to document the fact it not only assigns the unit
but may also create a brand new machine to accomplish the task it was
given.

https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode250
state/unit.go:250: case noUnusedMachines:
if err != noUnusedMachines {
     return err
}

https://codereview.appspot.com/6248076/

« Back to merge proposal