Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2412)

Issue 6248076: add state.AssignmentPolicy type and state.AssignUnit func

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years ago by fwereade
Modified:
9 years ago
Reviewers:
mp+108158
Visibility:
Public.

Description

add state.AssignmentPolicy type and state.AssignUnit func https://code.launchpad.net/~fwereade/juju/go-place-unit/+merge/108158 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8

Patch Set 2 : add state.PlacementPolicy type, and state.Unit.Place method #

Total comments: 4

Patch Set 3 : add state.AssignmentPolicy type and state.AssignUnit func #

Unified diffs Side-by-side diffs Delta from patch set Stats (+101 lines, -19 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M state/state_test.go View 1 6 chunks +62 lines, -18 lines 0 comments Download
M state/unit.go View 1 2 4 chunks +37 lines, -1 line 0 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
9 years ago (2012-05-31 13:32:00 UTC) #1
niemeyer
Implementation looks nice. I have a couple of API cleanup suggestions for you to consider. ...
9 years ago (2012-06-05 19:58:07 UTC) #2
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 ...
9 years ago (2012-06-06 10:18:35 UTC) #3
niemeyer
LGTM, thanks. It'd be nice to move the helper function back where it was (or ...
9 years ago (2012-06-06 10:40:04 UTC) #4
fwereade
9 years ago (2012-06-06 10:52:46 UTC) #5
*** 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b