Skip to content

add cluster events details in swagger.yml#34035

Merged
vieux merged 1 commit into
moby:masterfrom
allencloud:add-cluster-event-details-in-swagger
Jul 11, 2017
Merged

add cluster events details in swagger.yml#34035
vieux merged 1 commit into
moby:masterfrom
allencloud:add-cluster-event-details-in-swagger

Conversation

@allencloud

Copy link
Copy Markdown
Contributor

Signed-off-by: allencloud [email protected]

PR #32421 introduces the cluster events. While this part has not updated in the docs and swagger.

- What I did

  1. add cluster events details in swagger.yml

- How I did it

- How to verify it

- Description for the changelog

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

@allencloud
allencloud force-pushed the add-cluster-event-details-in-swagger branch from f4e669a to cc51279 Compare July 10, 2017 09:38
@vieux

vieux commented Jul 10, 2017

Copy link
Copy Markdown
Contributor

LGTM ping @thaJeztah

Comment thread api/swagger.yaml Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

typo: updte

@thaJeztah thaJeztah left a comment

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.

LGTM after the typo was fixed

Perhaps as a follow-up, we should add enum's for the "type" and "action" fields, like

enum: ["tcp", "udp"]

Comment thread api/swagger.yaml 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, actually, can you change these to:

`create`, `connect`, `disconnect`, `destroy`, `update`, and `remove`

(each option in its own "code" block, instead of all in one code-block)

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 for the others

@allencloud
allencloud force-pushed the add-cluster-event-details-in-swagger branch from cc51279 to d6332e8 Compare July 11, 2017 01:16
@allencloud

allencloud commented Jul 11, 2017

Copy link
Copy Markdown
Contributor Author

Currently updated the typo. Yeah, I think this PR also dependent on PR #34032 . If PR #34032 is OK to merge, I will add config in this PR and do the enum thing. WDYT? @thaJeztah

@thaJeztah thaJeztah left a comment

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.

LGTM see below

ping @vdemeester PTAL

@thaJeztah

Copy link
Copy Markdown
Member

oh! looks like there's an issue with the file;

01:17:28 api/swagger.yaml
01:17:28   5754:48   error    trailing spaces  (trailing-spaces)

@thaJeztah thaJeztah left a comment

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.

LGTM after the trailing space was removed 👍

Comment thread api/swagger.yaml 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.

There's a trailing space here 😅

@allencloud
allencloud force-pushed the add-cluster-event-details-in-swagger branch from d6332e8 to f596fb7 Compare July 11, 2017 03:54
@allencloud

Copy link
Copy Markdown
Contributor Author

Oh, my bad. Updated now. @thaJeztah @vdemeester

@thaJeztah thaJeztah left a comment

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.

LGTM (if green 😄)

@thaJeztah

Copy link
Copy Markdown
Member

experimental failing on #34051

@boaz0 boaz0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 2 🍰

@vieux
vieux merged commit 6978a6e into moby:master Jul 11, 2017
@allencloud
allencloud deleted the add-cluster-event-details-in-swagger branch July 11, 2017 09:18
@thaJeztah thaJeztah added this to the 17.06.0 milestone Jul 19, 2017
@thaJeztah thaJeztah added the area/api API label Jul 21, 2017
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.

6 participants