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

update events related doc and swagger.yml #29906

Merged
merged 1 commit into from
Feb 8, 2017

Conversation

allencloud
Copy link
Contributor

@allencloud allencloud commented Jan 5, 2017

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

I found that about docker events there are something which are stale. This PR adds some missing details and updates some stale examples.

- What I did

  1. add missing events in doc and swagger.yml
  2. update some stale examples

- How I did it

- How to verify it

- Description for the changelog

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

Copy link
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.

left some comments

@@ -30,7 +30,7 @@ Options:

Docker containers report the following events:

attach, commit, copy, create, destroy, detach, die, exec_create, exec_detach, exec_start, export, health_status, kill, oom, pause, rename, resize, restart, start, stop, top, unpause, update
archive-path, attach, checkpoint, commit, copy, create, destroy, detach, die, exec_create, exec_detach, exec_start, export, extract-to-dir, health_status, kill, oom, pause, rename, resize, restart, start, stop, top, unpause, update
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for doing this @allencloud; the checkpoint events should probably not be documented for now, because they're still experimental (I don't think we document experimental events, because they can still change so are not really part of the "specs")

I'm having a discussion right now with @vdemeester about the archive-path and extract-to-dir events. They appear to be implemented as part of #13171 (c32dde5), but I think they're really "odd".

  • They don't follow the same naming conventions as other events (dash instead of underscore)
  • It's unclear what their purpose is (they're related to copy - if they're important, they should probably be renamed to something like copy_start, copy_complete)

I'm not sure what the intended purpose was for these events; if it's to collect metrics, then maybe they should be moved to the Prometheus API.

Let's not document these for now, and not "bake" them into the API, I think these need discussion

ping @jlhawn ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

When the changes to docker cp were made a year-and-a-half ago, there was an existing API endpoint named copy which emitted the copy event. Since the changes added two new API endpoints, I added these two new events so that the new API endpoints still produced their own events. Sorry, I didn't realize at the time that they violated the naming convention (or that there even was one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your detailed explanation. @jlhawn Happy to see the original thought on implementation.
Then though it is work one and half years ago, what do you think is a better way to continue the following things, revert to the right naming thing or keep this in documents? @thaJeztah @jlhawn

2015-05-12T15:52:12.999999999Z07:00 container stop 4386fb97867d (image=ubuntu-1:14.04)
2015-05-12T15:53:45.999999999Z07:00 container die 7805c1d35632 (image=redis:2.8)
2015-05-12T15:54:03.999999999Z07:00 container stop 7805c1d35632 (image=redis:2.8)
2017-01-05T00:35:58.859401177+08:00 container create 0fdb48addc82871eb34eb23a847cfd033dedd1a0a37bef2e6d9eb3870fc7ff37 (image=alpine:latest, name=test)
Copy link
Member

Choose a reason for hiding this comment

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

wonder if we should truncate the full ID to make the documentation more readable, e.g.

2017-01-05T00:35:58.859401177+08:00 container create 0fdb...ff37 (image=alpine:latest, name=test)

I think that's ok to do as long as we mention that container ID's are shortened for readability

Copy link
Member

Choose a reason for hiding this comment

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

It may be worth showing the actual container-id that's printed after docker create, to relate the "id" in the examples with the "id" of the container

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback @thaJeztah
I did some change. And please help me check since I was wondering if I follow your point correctly. 😄

Signed-off-by: allencloud <allen.sun@daocloud.io>
@allencloud
Copy link
Contributor Author

Hi @thaJeztah
I am wondering how we could make this move forward. Or anything else I could do for this? 😄

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 27, 2017

ping @thaJeztah

Copy link
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.

ugh had my review in "pending"

LGTM

@thaJeztah thaJeztah merged commit dea528e into moby:master Feb 8, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 8, 2017
@thaJeztah thaJeztah modified the milestones: 1.13.1, 1.14.0 Feb 8, 2017
@allencloud allencloud deleted the update-events-related-things branch February 9, 2017 01:15
@thaJeztah thaJeztah modified the milestones: 1.13.2, 1.13.1 Feb 17, 2017
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Feb 17, 2017
…hings

update events related doc and swagger.yml
(cherry picked from commit dea528e)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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