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

Add a "build" event #15216

Closed
wants to merge 1 commit into from
Closed

Add a "build" event #15216

wants to merge 1 commit into from

Conversation

duglin
Copy link
Contributor

@duglin duglin commented Jul 31, 2015

Closes #15211

Signed-off-by: Doug Davis dug@us.ibm.com

@duglin duglin changed the title Adds a "build" event Add a "build" event Jul 31, 2015
@duglin
Copy link
Contributor Author

duglin commented Jul 31, 2015

hmmm why did gordon add "group/distribution" ?

@vdemeester
Copy link
Member

@duglin I think it if there is a modification on builder package it does (amongst other) — https://github.com/docker/leeroy/blob/master/github/pull_request.go#L72 😉.

Docker builds report:

build

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thaJeztah done - thanks for the pointers.

Closes moby#15211

Signed-off-by: Doug Davis <dug@us.ibm.com>
@duglin
Copy link
Contributor Author

duglin commented Aug 3, 2015

ping @cpuguy83 @LK4D4

@cpuguy83
Copy link
Member

cpuguy83 commented Aug 3, 2015

I don't see the purpose of adding an even that simply says "build".
This isn't really actionable in anyway and doesn't really provide any information at all.

@duglin
Copy link
Contributor Author

duglin commented Aug 3, 2015

To me it provides context for the events that follow. Granted it would be nice if there were some sort of "context" ID that we could log to group all of the events together, but we don't have that yet.

@cpuguy83
Copy link
Member

cpuguy83 commented Aug 3, 2015

It may provide context, but really probably won't as there is no guarantee that what follows is for the build.
Maybe on a single user machine when the user does absolutely nothing else while building.

@duglin
Copy link
Contributor Author

duglin commented Aug 3, 2015

right which is why a context ID in each event would be nice. But in the meantime, it might be nice to know when someone asked for a build to happen. If someone can think of additional info to put in there I can add it.

@cpuguy83
Copy link
Member

cpuguy83 commented Aug 3, 2015

I'm not totally against a build event, but an empty one really doesn't seem useful.

@duglin
Copy link
Contributor Author

duglin commented Aug 3, 2015

Yea, tried to find some other info to put in there but couldn't think of anything. :-(

@tiborvass
Copy link
Contributor

How would this make any sense, when builder is client side and just creates containers, and calls copy, etc. ? Then there is no more notion of "build".

@duglin
Copy link
Contributor Author

duglin commented Aug 3, 2015

true but as long as there's a daemon-side build command I think it makes sense from an admin's perspective to know what their users are doing.

@moysesb
Copy link
Contributor

moysesb commented Aug 4, 2015

Forgive me if I go out in a slight tangent here, but this PR touches on some issues I'm having while creating a client for docker's remote api. Speaking specifically about the build system, one of the things I'm really missing is a richer, more deliberate event system. I know that others have had the same problem where the three steps of building, namely 1) issuing the build command, 2) following its progress and 3) being notified at the end either with an error or an image id, are too tightly coupled with one another. There's no easy, formal way of separating these three phases and composing them at will (see for example this issue on docker-py).

From my poing of view as a client of the API what I'd like to do is:

  1. Issue a build command and receive an ACCEPTED response with an URL for the progress stream (say, http://dockerserver/build/<ticket>). An event would be generated, containing that URL.
  2. Optionally GET http://dockerserver/build/<ticket> and receive the streamed output from the builder. Other events are generated as the build progresses.
  3. When the build completes an event is generated, referring to the ticket above and with the id of the image created by the build process.

I know that this doesn't fit with the current command/event model so this really has no direct bearing on this PR; I just wanted to make the point that there's space and need for evolving the event system into a more fleshed out model.

@cpuguy83
Copy link
Member

Do you think it makes sense to close this until we can actually provide some better information to the event regarding the build?

@duglin
Copy link
Contributor Author

duglin commented Aug 18, 2015

@cpuguy83 I guess it depends on whether the fact that a build took place at all is interesting or not, even if we can't provide more info. I'm leaning more towards saying "yes" even if the data we can provide at this time isn't as complete as we'd like. Sort of like "is it a step in the right direction even if not the complete solution?"

Just brainstorming, but one option is to add two events, a "build-start" and a "build-end". Then the "build-end" could include the image ID, in the successful case. We could also include a unique ID on both events to help relate them in the case of multiple builds going on at once.

@calavera
Copy link
Contributor

Just brainstorming, but one option is to add two events, a "build-start" and a "build-end". Then the "build-end" could include the image ID, in the successful case. We could also include a unique ID on both events to help relate them in the case of multiple builds going on at once.

I like how all this sounds, can we have the code now? 😉

@duglin
Copy link
Contributor Author

duglin commented Aug 28, 2015

yep - finally done with weeks of travel, should be able to do this soon...

@tiborvass
Copy link
Contributor

@duglin

#15216 (comment)

true but as long as there's a daemon-side build command I think it makes sense from an admin's perspective to know what their users are doing.

But the users will not use the /build endpoint anymore.

@jessfraz
Copy link
Contributor

jessfraz commented Oct 8, 2015

I think this is moot since we are moving builder client side anyways, we will just have to remove it and go through that whole song and dance, sorry

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.

10 participants