Merge lp:~fwereade/pyjuju/go-place-unit into lp:pyjuju/go

[William Reade]

add state.AssignmentPolicy type and state.AssignUnit func

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

[William Reade (fwereade)]

Reviewers: mp+108158_code.launchpad.net,

Message:
Please take a look.

Description:
add state.PlacementPolicy type, and state.Unit.Place method

https://code.launchpad.net/~fwereade/juju/go-place-unit/+merge/108158

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6248076/

Affected files:
   A [revision details]
   M state/state_test.go
   M state/unit.go

[Gustavo Niemeyer (niemeyer)]

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/

[William Reade (fwereade)]

Please take a look.

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
On 2012/06/05 19:58:08, niemeyer wrote:
> Can we name this as AssignmentPolicy? I'd also be equally happy to go
to the
> other direction and name methods as PlaceInMachine etc.

Assignment SGTM

https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode30
state/unit.go:30: PlaceUnassigned PlacementPolicy = "unassigned"
On 2012/06/05 19:58:08, niemeyer wrote:
> It'd be nice to have a sentence on top of each explaining their
meaning.

Done.

https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode241
state/unit.go:241: func (u *Unit) Place(policy PlacementPolicy) (err
error) {
On 2012/06/05 19:58:08, niemeyer wrote:
> 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.

Done.

https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode250
state/unit.go:250: case noUnusedMachines:
On 2012/06/05 19:58:08, niemeyer wrote:
> if err != noUnusedMachines {
> return err
> }

D'oh! Done.

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

[Gustavo Niemeyer (niemeyer)]

LGTM, thanks. It'd be nice to move the helper function back where it was
(or around where it was, anyway).

https://codereview.appspot.com/6248076/diff/5001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6248076/diff/5001/state/state_test.go#newcode1062
state/state_test.go:1062: func (s *StateSuite) TestAddRelation(c *C) {
This shouldn't be here, I suspect.

https://codereview.appspot.com/6248076/diff/5001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6248076/diff/5001/state/util.go#newcode26
state/util.go:26: func AssignUnit(st *State, u *Unit, policy
AssignmentPolicy) (err error) {
It's fine to have this as a function in the place where it was. It's a
much better spot than here, as it had all the logic surrounding it just
a glance away.

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

[lp:~fwereade/pyjuju/go-place-unit]

197. By William Reade on 2012-06-06

address review

[William Reade (fwereade)]

*** Submitted:

add state.AssignmentPolicy type and state.AssignUnit func

R=niemeyer
CC=
https://codereview.appspot.com/6248076

https://codereview.appspot.com/6248076/diff/5001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6248076/diff/5001/state/state_test.go#newcode1062
state/state_test.go:1062: func (s *StateSuite) TestAddRelation(c *C) {
On 2012/06/06 10:40:04, niemeyer wrote:
> This shouldn't be here, I suspect.

Ignored per IRC :).

https://codereview.appspot.com/6248076/diff/5001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6248076/diff/5001/state/util.go#newcode26
state/util.go:26: func AssignUnit(st *State, u *Unit, policy
AssignmentPolicy) (err error) {
On 2012/06/06 10:40:04, niemeyer wrote:
> It's fine to have this as a function in the place where it was. It's a
much
> better spot than here, as it had all the logic surrounding it just a
glance
> away.

Done.

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