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

Issue 8561046: cmd/juju: status displays machine errors (Closed)

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

Description

cmd/juju: status displays machine errors This makes the juju status command behave in a more py-juju compatible way, displaying the following additional information for each machine: agent-state: [whatever machine.Status() reports] agent-state-info: [machine.Status().Info, if set] "instance-state" is not possible to support right now, so I didn't implement it, but filed a lp:1167441 bug. Also, when the agent is detected as "down", the agent-state-info shows the original status and the info (if set). Finally, there is a fix for the machiner to handle SetAgentAlive() and testing it correctly. https://code.launchpad.net/~dimitern/juju-core/029-juju-status-displays-machine-state-and-errors/+merge/158135 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 4

Patch Set 2 : cmd/juju: status displays machine errors #

Patch Set 3 : cmd/juju: status displays machine errors #

Total comments: 9

Patch Set 4 : cmd/juju: status displays machine errors #

Patch Set 5 : cmd/juju: status displays machine errors #

Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -36 lines) Patch
A [revision details] View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/status.go View 1 2 3 3 chunks +34 lines, -14 lines 0 comments Download
M cmd/juju/status_test.go View 1 2 3 4 11 chunks +127 lines, -17 lines 0 comments Download
M state/api/params/status.go View 1 chunk +1 line, -1 line 0 comments Download
M worker/machiner/machiner.go View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M worker/machiner/machiner_test.go View 1 2 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 9
dimitern
Please take a look.
10 years, 11 months ago (2013-04-10 14:58:55 UTC) #1
rog
LGTM with one trivial https://codereview.appspot.com/8561046/diff/1/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8561046/diff/1/cmd/juju/status.go#newcode161 cmd/juju/status.go:161: if len(info) > 0 { ...
10 years, 11 months ago (2013-04-10 15:23:03 UTC) #2
dimitern
Please take a look. https://codereview.appspot.com/8561046/diff/1/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8561046/diff/1/cmd/juju/status.go#newcode161 cmd/juju/status.go:161: if len(info) > 0 { ...
10 years, 11 months ago (2013-04-10 16:29:16 UTC) #3
fwereade
Needs a bit of work: https://codereview.appspot.com/8561046/diff/1/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8561046/diff/1/cmd/juju/status.go#newcode157 cmd/juju/status.go:157: r["agent-state"] = "not-started" agent-state ...
10 years, 11 months ago (2013-04-10 16:58:25 UTC) #4
fwereade
On 2013/04/10 16:58:25, fwereade wrote: > agent-state needs to hold status, and agent-status-info to hold ...
10 years, 11 months ago (2013-04-10 16:59:08 UTC) #5
dimitern
On 2013/04/10 16:58:25, fwereade wrote: > Needs a bit of work: > > https://codereview.appspot.com/8561046/diff/1/cmd/juju/status.go > ...
10 years, 11 months ago (2013-04-11 11:14:50 UTC) #6
dimitern
Please take a look.
10 years, 11 months ago (2013-04-11 11:23:08 UTC) #7
fwereade
LGTM https://codereview.appspot.com/8561046/diff/11001/cmd/juju/status.go File cmd/juju/status.go (right): https://codereview.appspot.com/8561046/diff/11001/cmd/juju/status.go#newcode141 cmd/juju/status.go:141: // The following logic is ported from Python ...
10 years, 11 months ago (2013-04-11 11:35:11 UTC) #8
dimitern
10 years, 11 months ago (2013-04-11 12:00:27 UTC) #9
*** Submitted:

cmd/juju: status displays machine errors

This makes the juju status command behave in a
more py-juju compatible way, displaying the
following additional information for each
machine:
  agent-state: [whatever machine.Status() reports]
  agent-state-info: [machine.Status().Info, if set]

"instance-state" is not possible to support right now,
so I didn't implement it, but filed a lp:1167441 bug.
Also, when the agent is detected as "down", the
agent-state-info shows the original status and the
info (if set).

Finally, there is a fix for the machiner to handle
SetAgentAlive() and testing it correctly.

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

https://codereview.appspot.com/8561046/diff/11001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8561046/diff/11001/cmd/juju/status.go#newcode141
cmd/juju/status.go:141: // The following logic is ported from Python juju status
command.
On 2013/04/11 11:35:11, fwereade wrote:
> No longer accurate

Removed.

https://codereview.appspot.com/8561046/diff/11001/cmd/juju/status.go#newcode161
cmd/juju/status.go:161: status = params.MachineDown
On 2013/04/11 11:35:11, fwereade wrote:
> This duplicated chunk makes me feel that maybe we should have a single Status
> type. Opinion?

Let's not do this for now. We might find cases where different status types are
useful.

https://codereview.appspot.com/8561046/diff/11001/cmd/juju/status.go#newcode242
cmd/juju/status.go:242: info = string(status)
On 2013/04/11 11:35:11, fwereade wrote:
> parenthesize these too please

Done.

https://codereview.appspot.com/8561046/diff/11001/worker/machiner/machiner.go
File worker/machiner/machiner.go (left):

https://codereview.appspot.com/8561046/diff/11001/worker/machiner/machiner.go...
worker/machiner/machiner.go:60: mr.st.StartSync()
On 2013/04/11 11:35:11, fwereade wrote:
> Thanks for tidying this up.

Cheers.
Sign in to reply to this message.

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