Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show the unit leader in status. #6350

Merged
merged 2 commits into from Sep 30, 2016
Merged

Conversation

howbazaar
Copy link
Contributor

@howbazaar howbazaar commented Sep 28, 2016

Addresses http://pad.lv/1620063.

Whether the unit is the leader or not is now returned in the FullStatus. The leader is indicated in tabular output with an * at the end of the unit name. YAML and JSON have a map value called "leader" that is only added when it is true.

The addition of the field into the FullStatus output does not need any facade version bump. It is optional, and the code handles it if it is missing by just not indicating a leader. This has been confirmed by running the new client against an old server.

QA

$ juju bootstrap status-test lxd
$ juju deploy ubuntu -n 2
# wait
$ juju status
juju status
MODEL    CONTROLLER   CLOUD/REGION   VERSION
default  status-test  lxd/localhost  2.0-rc2.1

APP     VERSION  STATUS  SCALE  CHARM   STORE       REV  OS      NOTES
ubuntu  16.04    active      2  ubuntu  jujucharms    8  ubuntu  

UNIT       WORKLOAD  AGENT  MACHINE  PUBLIC-ADDRESS  PORTS  MESSAGE
ubuntu/0*  active    idle   0        10.250.171.239         ready
ubuntu/1   active    idle   1        10.250.171.225         ready

MACHINE  STATE    DNS             INS-ID         SERIES  AZ
0        started  10.250.171.239  juju-3ae63c-0  xenial  
1        started  10.250.171.225  juju-3ae63c-1  xenial  

Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd prefer if we didn't display the * for single-unit applications.

@@ -89,6 +89,7 @@ type UnitStatus struct {
PublicAddress string `json:"public-address"`
Charm string `json:"charm"`
Subordinates map[string]UnitStatus `json:"subordinates"`
Leader bool `json:"leader"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omitempty, to save a few bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had that and took it off. Will put it back

@@ -1814,7 +1826,7 @@ var statusTests = []testCase{

// scoped on wordpress/0
scopedExpect{
"subordinates scoped on logging",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was correct? The subordinate is scoped to the logging relation of the wordpress application?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a copy/paste error from a previous test. Updated the comment of the test to match the args passed to status, and the comment just before the 'scopedExpect'

@@ -177,6 +177,9 @@ func FormatTabular(writer io.Writer, forceColor bool, value interface{}) error {
if agentDoing != "" {
message = fmt.Sprintf("(%s) %s", agentDoing, message)
}
if u.Leader {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we don't display this if there's only one unit? it's noisy/uninteresting in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no nice way to not do this. Discussed on the bug with sabdfl, and agreed that at least for the first cut, we always show it.

@howbazaar
Copy link
Contributor Author

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Sep 30, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 4761e14 into juju:master Sep 30, 2016
@howbazaar howbazaar deleted the status-unit-leader branch September 30, 2016 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants