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

Add `--format` for `docker node ls` #30424

Merged
merged 2 commits into from Apr 5, 2017

Conversation

@yongtang
Member

yongtang commented Jan 24, 2017

- What I did
This fix tries to address the comment #30376 (comment) where it was not possible to specify --format for docker node ls. The --format flag is a quite useful flag that could be used in many places such as completion.

- How I did it

This fix implements --format for docker node ls and add nodesFormat in config.json so that it is possible to specify the output when docker node ls is invoked.

Related documentations have been updated.

- How to verify it

A set of unit tests have been added.

- Description for the changelog

Add --format for docker node ls and add nodesFormat in config.json so that it is possible to specify the output when docker node ls is invoked.

- A picture of a cute animal (not mandatory but encouraged)

enhanced-1301-1438290165-1

This fix is related to #30376.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang
Member

yongtang commented Jan 24, 2017

if err != nil {
t.Fatal(err)
}
for i, line := range strings.Split(strings.TrimSpace(out.String()), "\n") {

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Jan 25, 2017

Member

We could deduplicate similar code in cli/command/formatter/*_test.go. But it can be another PR in the future.

@AkihiroSuda

AkihiroSuda Jan 25, 2017

Member

We could deduplicate similar code in cli/command/formatter/*_test.go. But it can be another PR in the future.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda
Member

AkihiroSuda commented Jan 25, 2017

SGTM

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Jan 25, 2017

Member

Thanks @yongtang. Your're doing great work!

Member

albers commented Jan 25, 2017

Thanks @yongtang. Your're doing great work!

@vdemeester

Design LGTM 👼
I would have added a case in the node_list_test.go tests, but otherwise looks good !

Show outdated Hide outdated cli/internal/test/cli.go
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 27, 2017

Member

Thanks @vdemeester for the review. The PR has been updated with additional unit tests added in node/list_test.go. Please take a look.

Member

yongtang commented Jan 27, 2017

Thanks @vdemeester for the review. The PR has been updated with additional unit tests added in node/list_test.go. Please take a look.

@vdemeester

LGTM 🐯

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 6, 2017

Member

Thanks @AkihiroSuda. The PR has been rebased and updated. Now an additional note has been added:

 
+> **Note:**
+> If the `ID` field of the node is followed by a `*` (e.g., `e216jshn25ckzbvmwlnh5jr3g *`)
+> in the above example output, then this node is also the node of the current docker daemon.
+
 
 ## Filtering
 
@@ -120,6 +125,35 @@ ID                           HOSTNAME        STATUS  AVAILABILITY  MANAGER STATU
 e216jshn25ckzbvmwlnh5jr3g *  swarm-manager1  Ready   Active        Leader

Please take a look and let me know if there are any issues.

Member

yongtang commented Feb 6, 2017

Thanks @AkihiroSuda. The PR has been rebased and updated. Now an additional note has been added:

 
+> **Note:**
+> If the `ID` field of the node is followed by a `*` (e.g., `e216jshn25ckzbvmwlnh5jr3g *`)
+> in the above example output, then this node is also the node of the current docker daemon.
+
 
 ## Filtering
 
@@ -120,6 +125,35 @@ ID                           HOSTNAME        STATUS  AVAILABILITY  MANAGER STATU
 e216jshn25ckzbvmwlnh5jr3g *  swarm-manager1  Ready   Active        Leader

Please take a look and let me know if there are any issues.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Feb 20, 2017

Member

@thaJeztah PTAL about the * symbol?

@yongtang please rebase?

Member

AkihiroSuda commented Feb 20, 2017

@thaJeztah PTAL about the * symbol?

@yongtang please rebase?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 20, 2017

Member

@AkihiroSuda Thanks. The PR has been rebased.

@AkihiroSuda @vdemeester @thaJeztah I think to add * as an individual column, we might be able to achieve that by having a boolean placeholder with empty header name and either "*" or "" as the output (true -> "*" and false -> ""). Though I would like to get some feedback before moving forward.

Member

yongtang commented Feb 20, 2017

@AkihiroSuda Thanks. The PR has been rebased.

@AkihiroSuda @vdemeester @thaJeztah I think to add * as an individual column, we might be able to achieve that by having a boolean placeholder with empty header name and either "*" or "" as the output (true -> "*" and false -> ""). Though I would like to get some feedback before moving forward.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 20, 2017

Member

@yongtang how does it look with an empty header ? (I would think it's ok to go that way 👼)

Member

vdemeester commented Feb 20, 2017

@yongtang how does it look with an empty header ? (I would think it's ok to go that way 👼)

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 20, 2017

Member

@vdemeester I just tried with empty/whitespace header " " locally. It does not look good however. The reason is that no matter what spacing is added to the header, the current template implementation will always use "\t" to separate the headers. Therefore the header row and the remaining row are mismatched.

Another option is to stick with "\t" delim but that looks ugly as well:

ID                                      HOSTNAME            STATUS              AVAILABILITY        MANAGER STATUS
nodeID1             *                   foobar_baz          Foo                 Drain               Leader
nodeID2                                 foobar_bar          Bar                 Active              Reachable		

There is a separate PR #30733 tracking the header delim of the template issue #28213 (comment) though. That PR may help address the separate 1-char column display (like " " or "*").

Member

yongtang commented Feb 20, 2017

@vdemeester I just tried with empty/whitespace header " " locally. It does not look good however. The reason is that no matter what spacing is added to the header, the current template implementation will always use "\t" to separate the headers. Therefore the header row and the remaining row are mismatched.

Another option is to stick with "\t" delim but that looks ugly as well:

ID                                      HOSTNAME            STATUS              AVAILABILITY        MANAGER STATUS
nodeID1             *                   foobar_baz          Foo                 Drain               Leader
nodeID2                                 foobar_bar          Bar                 Active              Reachable		

There is a separate PR #30733 tracking the header delim of the template issue #28213 (comment) though. That PR may help address the separate 1-char column display (like " " or "*").

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Mar 2, 2017

Member

Rebased to fix merge conflict.

Member

yongtang commented Mar 2, 2017

Rebased to fix merge conflict.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 28, 2017

Collaborator

LGTM

Collaborator

tiborvass commented Mar 28, 2017

LGTM

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 28, 2017

Collaborator

Ping @thaJeztah @mstanleyjones @johndmulhausen

Collaborator

tiborvass commented Mar 28, 2017

Ping @thaJeztah @mstanleyjones @johndmulhausen

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 28, 2017

Member

I'm just thinking if .Self should be a Boolean to separate "presentation" from "data" (e.g. .IsCurrent); that way, a JSON format output would possibly make more sense, and users can pick their own "format". It does make the template a bit more difficult to use though, so open to input. Template would probably look something like;

table {{.ID}}{{if .IsCurrent}} * {{end}}\t{{.Hostname}}\t{{.Status}}\t{{.Availability}}\t{{.ManagerStatus}}

Given, we can already to {{if eq .Self "*"}} yay, I am "self"{{ end }}, so happy to hear if we're ok with the current implementation 😊

Member

thaJeztah commented Mar 28, 2017

I'm just thinking if .Self should be a Boolean to separate "presentation" from "data" (e.g. .IsCurrent); that way, a JSON format output would possibly make more sense, and users can pick their own "format". It does make the template a bit more difficult to use though, so open to input. Template would probably look something like;

table {{.ID}}{{if .IsCurrent}} * {{end}}\t{{.Hostname}}\t{{.Status}}\t{{.Availability}}\t{{.ManagerStatus}}

Given, we can already to {{if eq .Self "*"}} yay, I am "self"{{ end }}, so happy to hear if we're ok with the current implementation 😊

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 4, 2017

Member

ping @yongtang, I was briefly discussing this, and perhaps it's best to use the boolean approach if possible; would you be able to have a look if that works?

Member

thaJeztah commented Apr 4, 2017

ping @yongtang, I was briefly discussing this, and perhaps it's best to use the boolean approach if possible; would you be able to have a look if that works?

Show outdated Hide outdated docs/reference/commandline/cli.md
Show outdated Hide outdated docs/reference/commandline/cli.md
Show outdated Hide outdated docs/reference/commandline/node_ls.md

yongtang added some commits Jan 24, 2017

Add `--format` for `docker node ls`
This fix tries to address the comment #30376 (comment)
where it was not possible to specify `--format` for `docker node ls`. The `--format` flag
is a quite useful flag that could be used in many places such as completion.

This fix implements `--format` for `docker node ls` and add `nodesFormat` in config.json
so that it is possible to specify the output when `docker node ls` is invoked.

Related documentations have been updated.

A set of unit tests have been added.

This fix is related to #30376.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Add hidden placeholder of `.Self` for `docker node ls --format`
This commit adds a hidden placeholder of `.Self` for
`docker node ls --format` so that if the node is the same
as the current docker daemon, then a `*` is outputed.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Apr 4, 2017

Member

Thanks @thaJeztah. The PR has been updated. Now the default format is

defaultNodeTableFormat = "table {{.ID}} {{if .Self}}*{{else}} {{ end }}\t{{.Hostname}}\t{{.Status}}\t{{.Availability}}\t{{.ManagerStatus}}"

which should achieve the same effect while at the same time allows .Self to be boolean. Please take a look.

Member

yongtang commented Apr 4, 2017

Thanks @thaJeztah. The PR has been updated. Now the default format is

defaultNodeTableFormat = "table {{.ID}} {{if .Self}}*{{else}} {{ end }}\t{{.Hostname}}\t{{.Status}}\t{{.Availability}}\t{{.ManagerStatus}}"

which should achieve the same effect while at the same time allows .Self to be boolean. Please take a look.

@mistyhacks

Docs LGTM

@vdemeester

LGTM 🦁

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 5, 2017

Member

Heh, found a funny one. Not sure yet if it's a "bug" or just a bad template;

docker node ls --format 'table {{.ID}} {{if not .Self}}*{{else}} {{ end }}\t{{.Hostname}}'
ID *                          HOSTNAME
ylm27u3p0wo04sdluj2ikigtf     6be31c5d1969

if not .Self is also evaluated in the "header" row, causing an * to be rendered there

Don't think that's a show stopper, but wanted to leave it as a comment in case someone has thoughts on how to (and if) to address 😄

Member

thaJeztah commented Apr 5, 2017

Heh, found a funny one. Not sure yet if it's a "bug" or just a bad template;

docker node ls --format 'table {{.ID}} {{if not .Self}}*{{else}} {{ end }}\t{{.Hostname}}'
ID *                          HOSTNAME
ylm27u3p0wo04sdluj2ikigtf     6be31c5d1969

if not .Self is also evaluated in the "header" row, causing an * to be rendered there

Don't think that's a show stopper, but wanted to leave it as a comment in case someone has thoughts on how to (and if) to address 😄

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 5, 2017

Member

This is really nice to have with the boolean 👍 thanks!

docker node ls --format '{{json .}}' | jq .
{
  "Availability": "Active",
  "Hostname": "6be31c5d1969",
  "ID": "ylm27u3p0wo04sdluj2ikigtf",
  "ManagerStatus": "Leader",
  "Self": true,
  "Status": "Ready"
}
Member

thaJeztah commented Apr 5, 2017

This is really nice to have with the boolean 👍 thanks!

docker node ls --format '{{json .}}' | jq .
{
  "Availability": "Active",
  "Hostname": "6be31c5d1969",
  "ID": "ylm27u3p0wo04sdluj2ikigtf",
  "ManagerStatus": "Leader",
  "Self": true,
  "Status": "Ready"
}
@thaJeztah

LGTM, thanks so much for doing this (and bearing with my nits) 👍

@thaJeztah thaJeztah merged commit 833f1f4 into moby:master Apr 5, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32528 has succeeded
Details
janky Jenkins build Docker-PRs 41136 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1310 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12247 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1142 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 5, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Apr 5, 2017

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Apr 5, 2017

Member

🎉

Member

vdemeester commented Apr 5, 2017

🎉

@yongtang yongtang deleted the yongtang:30376-node-ls-format branch Apr 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment