Remove engine.Job from Builder. by calavera · Pull Request #12457 · moby/moby

calavera · 2015-04-16T21:27:56Z

Part of #12151.

LK4D4 · 2015-04-16T21:34:27Z

@calavera Looks like panic

calavera · 2015-04-16T21:55:18Z

yeah, json deserialization 🙀

duglin · 2015-04-16T21:56:32Z

api/server/server.go

I'd prefer not to call this 'job' - it would be better to just call it "buildConfig" or something.

Yeah, I'm too for calling it buildConfig. Was very confused to read diff :)

agreed, it's still there because I used it as a reference to know what I had to change 😄

calavera · 2015-04-20T17:54:47Z

It looks like errors were not actually serialized to stdout either but just returned as err. Fixed it.

calavera · 2015-04-20T18:36:26Z

I've rebased this with master.

LK4D4 · 2015-04-20T19:25:35Z

@calavera Janky still failing :(

calavera · 2015-04-20T19:44:17Z

Yeah, it really really wants to check errors in the output and also in the returned err. Reverting Stdout to be an engine stream 😔

LK4D4 · 2015-04-20T19:46:51Z

@calavera Could you point me to place where it checks those errors?

calavera · 2015-04-20T19:48:03Z

Yeah, we need this condition:

https://github.com/docker/docker/pull/12457/files#diff-44a527ea72ea9572e1cf71c7f1e2a0f0R1338

Some errors are in the output, in that case we don't return err.

LK4D4 · 2015-04-20T19:53:21Z

I don't understand why we need this trick at all.

calavera · 2015-04-20T19:56:56Z

I think if we return the err, the api considers that there has been an error inside docker, but that's not necessarily correct. If the error was caused by a bad Dockerfile, it's a problem in the client code. In that case we want to ignore the error and return the stream from the builder.

LK4D4 · 2015-04-20T20:01:48Z

I think I know - it is detection of sending 200 OK on first write to responsewriter. Better to write simple wrapper around w and remove engine from here forever.

LK4D4 · 2015-04-20T20:24:11Z

Also, if we'll merge #12564 it will be possible to remove engine from builder totally :) but maybe in other PR.

Signed-off-by: David Calavera <[email protected]>

calavera · 2015-04-20T21:21:02Z

@LK4D4 well, it turns out that we already have a wrapper around the ResponseWriter, the WriteFlusher. I've added a flag to know when the stream has been flushed. That way we still can control the writer status without introducing a new wrapper. Let me know what you think.

LK4D4 · 2015-04-20T21:23:12Z

api/server/server.go

Will be a race. Should be protected by mutex. Maybe method Flushed()?

That way we can know when the stream has been flushed. Signed-off-by: David Calavera <[email protected]>

calavera · 2015-04-20T22:25:12Z

green again 🎉

LK4D4 · 2015-04-20T23:49:29Z

LGTM

jessfraz · 2015-04-21T00:27:14Z

LGTM

Remove engine.Job from Builder.

GordonTheTurtle added status/0-triage dco/yes and removed dco/no labels Apr 16, 2015

calavera force-pushed the remove_engine_job_builder branch from d3c7a31 to 368dde3 Compare April 16, 2015 21:29

GordonTheTurtle added dco/yes and removed dco/no labels Apr 16, 2015

duglin reviewed Apr 16, 2015
View reviewed changes

calavera force-pushed the remove_engine_job_builder branch from 368dde3 to 3f8c8e1 Compare April 16, 2015 22:21

GordonTheTurtle added dco/yes and removed dco/no labels Apr 16, 2015

calavera force-pushed the remove_engine_job_builder branch from 3f8c8e1 to 0c51f95 Compare April 16, 2015 22:44

GordonTheTurtle added dco/yes and removed dco/no labels Apr 16, 2015

cpuguy83 added status/2-code-review and removed status/0-triage labels Apr 16, 2015

calavera force-pushed the remove_engine_job_builder branch from 0c51f95 to faedd61 Compare April 17, 2015 17:02

GordonTheTurtle added dco/yes and removed dco/no labels Apr 17, 2015

calavera force-pushed the remove_engine_job_builder branch from faedd61 to 1edec46 Compare April 17, 2015 17:09

GordonTheTurtle added dco/yes and removed dco/no labels Apr 17, 2015

calavera mentioned this pull request Apr 17, 2015

remove job from pull and import #12490

Merged

calavera force-pushed the remove_engine_job_builder branch from 1edec46 to cc3cac2 Compare April 17, 2015 18:38

GordonTheTurtle added dco/yes and removed dco/no labels Apr 17, 2015

GordonTheTurtle added the dco/yes label Apr 20, 2015

calavera force-pushed the remove_engine_job_builder branch from d1aab9d to beb813e Compare April 20, 2015 18:35

GordonTheTurtle added dco/yes and removed dco/no labels Apr 20, 2015

calavera force-pushed the remove_engine_job_builder branch from beb813e to 66dad95 Compare April 20, 2015 19:45

GordonTheTurtle added dco/yes and removed dco/no labels Apr 20, 2015

calavera added 2 commits April 20, 2015 13:37

Remove engine.Job references from builder.CmdBuild

9a2c009

Signed-off-by: David Calavera <[email protected]>

Remove engine.Job from builder.CmdBuildConfig.

ae40635

Signed-off-by: David Calavera <[email protected]>

calavera force-pushed the remove_engine_job_builder branch from 66dad95 to 701a868 Compare April 20, 2015 21:19

LK4D4 reviewed Apr 20, 2015
View reviewed changes

Add flusher check to utils.WriteFlusher.

3b05005

That way we can know when the stream has been flushed. Signed-off-by: David Calavera <[email protected]>

calavera force-pushed the remove_engine_job_builder branch from 701a868 to 3b05005 Compare April 20, 2015 21:25

jessfraz pushed a commit that referenced this pull request Apr 21, 2015

Merge pull request #12457 from calavera/remove_engine_job_builder

82f75df

Remove engine.Job from Builder.

jessfraz merged commit 82f75df into moby:master Apr 21, 2015

calavera deleted the remove_engine_job_builder branch April 21, 2015 16:28

Conversation

calavera commented Apr 16, 2015

Uh oh!

LK4D4 commented Apr 16, 2015

Uh oh!

calavera commented Apr 16, 2015

Uh oh!

duglin Apr 16, 2015

Choose a reason for hiding this comment

Uh oh!

LK4D4 Apr 17, 2015

Choose a reason for hiding this comment

Uh oh!

calavera Apr 17, 2015

Choose a reason for hiding this comment

Uh oh!

calavera commented Apr 20, 2015

Uh oh!

calavera commented Apr 20, 2015

Uh oh!

LK4D4 commented Apr 20, 2015

Uh oh!

calavera commented Apr 20, 2015

Uh oh!

LK4D4 commented Apr 20, 2015

Uh oh!

calavera commented Apr 20, 2015

Uh oh!

LK4D4 commented Apr 20, 2015

Uh oh!

calavera commented Apr 20, 2015

Uh oh!

LK4D4 commented Apr 20, 2015

Uh oh!

LK4D4 commented Apr 20, 2015

Uh oh!

calavera commented Apr 20, 2015

Uh oh!

LK4D4 Apr 20, 2015

Choose a reason for hiding this comment

Uh oh!

calavera Apr 20, 2015

Choose a reason for hiding this comment

Uh oh!

calavera commented Apr 20, 2015

Uh oh!

LK4D4 commented Apr 20, 2015

Uh oh!

jessfraz commented Apr 21, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants