Skip to content

change node ls and service ls api and docs#28192

Merged
vieux merged 1 commit intomoby:masterfrom
allencloud:fix-docs-issue-on-service-ls-api
Nov 10, 2016
Merged

change node ls and service ls api and docs#28192
vieux merged 1 commit intomoby:masterfrom
allencloud:fix-docs-issue-on-service-ls-api

Conversation

@allencloud
Copy link
Copy Markdown
Contributor

@allencloud allencloud commented Nov 9, 2016

fixes #28165

There are some places in docs that maybe need improvements.

  • command docker node ls;
  • command docker service ls description
  • api GET /services
  • api GET /nodes

All about filter description.

Also add more description about label filtering in docker node ls. related to #27231
Label filtering in docker node ls is looking at engine labels rather than node labels.

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

Signed-off-by: allencloud allen.sun@daocloud.io

@vdemeester
Copy link
Copy Markdown
Member

/cc @thaJeztah @mstanleyjones

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice catch on rejected

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

perhaps we should put this list in alphabetical order wdyt?

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! I left some comments/suggestions

Comment thread docs/reference/commandline/node_ls.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you keep these blank lines? having an empty line makes it easier for people to distinguish the command that was run, and the output (even though it's not actually printed with a blank line)

Comment thread docs/reference/commandline/node_ls.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here (blank line)

Comment thread docs/reference/commandline/node_ls.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a blank line after here (see my explanation above)

Comment thread docs/reference/commandline/node_ls.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a blank line after here (see my explanation above)

Comment thread docs/reference/commandline/node_ls.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should simplify, because this sentence, and instead add an example that shows a filter on (e.g.) label=somelabel=somevalue;

The `label` filter matches nodes based on engine labels. Node labels are currently not used for filtering.

@vdemeester wdyt?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should fix that btw, as it's not very logical to have nodes filter on engine properties

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here (alphabetical?)

@allencloud
Copy link
Copy Markdown
Contributor Author

done, updated. PTAL @thaJeztah @vdemeester

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

So sorry, @allencloud my mistake, should've seen that during my first round

Comment thread docs/reference/commandline/node_ls.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh! just recalled that in the new docs system, anchor tags need to be prepended with the document name (yup, it's annoying), so;

* [id](node_ls.md#ID)
* [label](node_ls.md#Label)
* [membership](node_ls.md#Membership)
* [name](node_ls.md#Name)
* [role](node_ls.md#Role)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here 😊

* [id](service_ls.md#ID)
* [label](service_ls.md#Label)
* [name](service_ls.md#Name)

Copy link
Copy Markdown
Contributor

@mdlinville mdlinville left a comment

Choose a reason for hiding this comment

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

LGTM with @thaJeztah's comments. Thanks!

@allencloud allencloud force-pushed the fix-docs-issue-on-service-ls-api branch from dd66133 to ceba47e Compare November 10, 2016 02:27
@allencloud
Copy link
Copy Markdown
Contributor Author

Oh, thanks for your advice. @thaJeztah @mstanleyjones
Now updated, PTAL.

Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@thaJeztah
Copy link
Copy Markdown
Member

Hold on, looks like the anchors should not have been changed to uppercase; anchors both on GitHub and docs.docker.com are lowercase (#id instead of #ID)

Signed-off-by: allencloud <allen.sun@daocloud.io>
@allencloud allencloud force-pushed the fix-docs-issue-on-service-ls-api branch from ceba47e to 48c3fce Compare November 10, 2016 18:51
@allencloud
Copy link
Copy Markdown
Contributor Author

Done, I did some improvement. But not sure if it is completed. @thaJeztah

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@vieux vieux merged commit a81385b into moby:master Nov 10, 2016
@allencloud allencloud deleted the fix-docs-issue-on-service-ls-api branch November 13, 2016 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docker remote API v1.24 Documentation discrepancy on GET /services

6 participants