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

Issue 8667043: various: unify machine and unit status types (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 7 months ago by dimitern
Modified:
8 years, 7 months ago
Reviewers:
mp+158411
Visibility:
Public.

Description

various: unify machine and unit status types Trivial change to simplify the code by unifying machine and unit status type to params.Status, rather than two separate types and separate list of constants. https://code.launchpad.net/~dimitern/juju-core/031-unify-machine-unit-status-types/+merge/158411 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 7

Patch Set 2 : various: unify machine and unit status types #

Unified diffs Side-by-side diffs Delta from patch set Stats (+208 lines, -237 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/builddb/main.go View 2 chunks +3 lines, -3 lines 0 comments Download
M cmd/juju/resolved_test.go View 1 chunk +1 line, -1 line 0 comments Download
M cmd/juju/status.go View 3 chunks +22 lines, -32 lines 0 comments Download
M cmd/juju/status_test.go View 7 chunks +10 lines, -10 lines 0 comments Download
M cmd/jujud/unit_test.go View 1 chunk +3 lines, -3 lines 0 comments Download
M state/api/params/params.go View 2 chunks +2 lines, -2 lines 0 comments Download
M state/api/params/status.go View 1 1 chunk +22 lines, -44 lines 0 comments Download
M state/apiserver/api_test.go View 1 2 chunks +2 lines, -2 lines 0 comments Download
M state/machine.go View 1 1 chunk +6 lines, -6 lines 0 comments Download
M state/machine_test.go View 6 chunks +26 lines, -26 lines 0 comments Download
M state/megawatcher.go View 3 chunks +4 lines, -4 lines 0 comments Download
M state/megawatcher_internal_test.go View 22 chunks +28 lines, -28 lines 0 comments Download
M state/service.go View 1 chunk +1 line, -1 line 0 comments Download
M state/state.go View 1 chunk +1 line, -1 line 0 comments Download
M state/status.go View 2 chunks +2 lines, -1 line 0 comments Download
M state/unit.go View 1 2 chunks +6 lines, -6 lines 0 comments Download
M state/unit_test.go View 2 chunks +21 lines, -21 lines 0 comments Download
M worker/machiner/machiner.go View 2 chunks +2 lines, -2 lines 0 comments Download
M worker/machiner/machiner_test.go View 4 chunks +4 lines, -4 lines 0 comments Download
M worker/provisioner/provisioner.go View 2 chunks +2 lines, -2 lines 0 comments Download
M worker/provisioner/provisioner_test.go View 1 chunk +1 line, -1 line 0 comments Download
M worker/uniter/filter_test.go View 1 chunk +1 line, -1 line 0 comments Download
M worker/uniter/modes.go View 5 chunks +5 lines, -5 lines 0 comments Download
M worker/uniter/uniter_test.go View 31 chunks +31 lines, -31 lines 0 comments Download

Messages

Total messages: 4
dimitern
Please take a look.
8 years, 7 months ago (2013-04-11 16:03:51 UTC) #1
fwereade
LGTM, nice
8 years, 7 months ago (2013-04-11 16:16:08 UTC) #2
rog
LGTM with a few trivials below. https://codereview.appspot.com/8667043/diff/1/cmd/juju/status.go File cmd/juju/status.go (left): https://codereview.appspot.com/8667043/diff/1/cmd/juju/status.go#oldcode151 cmd/juju/status.go:151: if status != ...
8 years, 7 months ago (2013-04-11 16:44:34 UTC) #3
dimitern
8 years, 7 months ago (2013-04-11 17:02:40 UTC) #4
*** Submitted:

various: unify machine and unit status types

Trivial change to simplify the code by unifying
machine and unit status type to params.Status,
rather than two separate types and separate
list of constants.

R=fwereade, rog
CC=
https://codereview.appspot.com/8667043

https://codereview.appspot.com/8667043/diff/1/state/api/params/status.go
File state/api/params/status.go (right):

https://codereview.appspot.com/8667043/diff/1/state/api/params/status.go#newc...
state/api/params/status.go:3: // Status represents the status of the unit,
machine or their agent.
On 2013/04/11 16:44:34, rog wrote:
> s/the/a/
> s/their/its/
> 
> or...
> how about:
> // Status represents the status of an entity.
> ?
> 
> that way we don't need to update all the comments if we
> start using it for something else.

I'll change it as suggested, change all the ones below to use "entity" and
mention in this one the entity could be a unit or a machine.

https://codereview.appspot.com/8667043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8667043/diff/1/state/machine.go#newcode507
state/machine.go:507: doc := statusDoc{status, info}
On 2013/04/11 16:44:34, rog wrote:
> i'd prefer to leave the fields explicit here, please.

I don't see a good reason, but I'll do it.

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

https://codereview.appspot.com/8667043/diff/1/state/unit.go#newcode453
state/unit.go:453: doc := statusDoc{status, info}
On 2013/04/11 16:44:34, rog wrote:
> please leave explicit fields.

Done.
Sign in to reply to this message.

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