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

Issue 6268050: subordinate service units can now be added to principal service units

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 10 months ago by fwereade
Modified:
11 years, 9 months ago
Reviewers:
mp+108575
Visibility:
Public.

Description

subordinate service units can now be added to principal service units This entails the following changes: * new Service.AddUnitSubordinateTo method, which requires a principal unit * topology.AddUnit now takes an additional principalKey string argument, which will be empty for a principal unit; * new topology.UnitPrincipalKey, which returns the principal unit key for a subordinate unit or an error for a principal unit; * new Unit.IsPrincipal method, which returns whether the unit is a principal unit; * checks to prevent subordinate units being assigned directly to machines. Most of the diff lines are a result of the change to topology.AddUnit; sorry for the noise. https://code.launchpad.net/~fwereade/juju/go-subordinate-units/+merge/108575 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : subordinate service units can now be added to principal service units #

Total comments: 8

Patch Set 3 : subordinate service units can now be added to principal service units #

Total comments: 2

Patch Set 4 : subordinate service units can now be added to principal service units #

Unified diffs Side-by-side diffs Delta from patch set Stats (+207 lines, -41 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M state/internal_test.go View 13 chunks +71 lines, -32 lines 0 comments Download
M state/service.go View 1 2 3 3 chunks +36 lines, -3 lines 0 comments Download
M state/state_test.go View 1 2 3 2 chunks +51 lines, -1 line 0 comments Download
M state/topology.go View 1 2 6 chunks +28 lines, -5 lines 0 comments Download
M state/unit.go View 1 2 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 7
fwereade
Please take a look.
11 years, 10 months ago (2012-06-04 14:21:15 UTC) #1
fwereade
Please take a look.
11 years, 10 months ago (2012-06-04 14:27:29 UTC) #2
TheMue
LGTM, only AddUnit(nil) reads a bit strange.
11 years, 10 months ago (2012-06-04 14:30:36 UTC) #3
niemeyer
Looks great. A suggestion for the API: https://codereview.appspot.com/6268050/diff/3001/state/service.go File state/service.go (right): https://codereview.appspot.com/6268050/diff/3001/state/service.go#newcode68 state/service.go:68: // AddUnit() ...
11 years, 9 months ago (2012-06-06 13:16:40 UTC) #4
fwereade
Please take a look. https://codereview.appspot.com/6268050/diff/3001/state/service.go File state/service.go (right): https://codereview.appspot.com/6268050/diff/3001/state/service.go#newcode68 state/service.go:68: // AddUnit() adds a new ...
11 years, 9 months ago (2012-06-06 14:42:35 UTC) #5
niemeyer
LGTM, but please address the panics before submitting. https://codereview.appspot.com/6268050/diff/10001/state/service.go File state/service.go (right): https://codereview.appspot.com/6268050/diff/10001/state/service.go#newcode123 state/service.go:123: panic("cannot ...
11 years, 9 months ago (2012-06-06 14:48:03 UTC) #6
fwereade
11 years, 9 months ago (2012-06-06 15:11:50 UTC) #7
*** Submitted:

subordinate service units can now be added to principal service units

This entails the following changes:

* new Service.AddUnitSubordinateTo method, which requires a principal unit
* topology.AddUnit now takes an additional principalKey string argument,
  which will be empty for a principal unit;
* new topology.UnitPrincipalKey, which returns the principal unit key for a
  subordinate unit or an error for a principal unit;
* new Unit.IsPrincipal method, which returns whether the unit is a principal
  unit;
* checks to prevent subordinate units being assigned directly to machines.

Most of the diff lines are a result of the change to topology.AddUnit;
sorry for the noise.

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6268050
Sign in to reply to this message.

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