Skip to content

Remove engine.Job from Builder. #12457

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.

Sign up for GitHub

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

Jump to bottom
Merged
jessfraz merged 3 commits into from
Apr 21, 2015
Merged

Remove engine.Job from Builder. #12457

jessfraz merged 3 commits into from
Apr 21, 2015

Conversation

@calavera
Copy link
Contributor

@calavera calavera commented Apr 16, 2015

Part of #12151.

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

LK4D4 commented Apr 16, 2015

@calavera Looks like panic

@calavera
Copy link
Contributor Author

calavera commented Apr 16, 2015

yeah, json deserialization 🙀

duglin
duglin reviewed Apr 16, 2015
View reviewed changes
api/server/server.go Outdated
Copy link
Contributor

@duglin duglin Apr 16, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@LK4D4 LK4D4 Apr 17, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@calavera calavera Apr 17, 2015

Choose a reason for hiding this comment

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

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

@calavera calavera force-pushed the remove_engine_job_builder branch from 368dde3 to 3f8c8e1 Compare April 16, 2015 22:21
@calavera calavera force-pushed the remove_engine_job_builder branch from 3f8c8e1 to 0c51f95 Compare April 16, 2015 22:44
@calavera calavera force-pushed the remove_engine_job_builder branch from 0c51f95 to faedd61 Compare April 17, 2015 17:02
@calavera calavera force-pushed the remove_engine_job_builder branch from faedd61 to 1edec46 Compare April 17, 2015 17:09
@calavera calavera mentioned this pull request Apr 17, 2015
Merged
@calavera calavera force-pushed the remove_engine_job_builder branch from 1edec46 to cc3cac2 Compare April 17, 2015 18:38
@calavera
Copy link
Contributor Author

calavera commented Apr 20, 2015

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

@calavera calavera force-pushed the remove_engine_job_builder branch from d1aab9d to beb813e Compare April 20, 2015 18:35
@calavera
Copy link
Contributor Author

calavera commented Apr 20, 2015

I've rebased this with master.

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 20, 2015

@calavera Janky still failing :(

@calavera
Copy link
Contributor Author

calavera commented Apr 20, 2015

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

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

LK4D4 commented Apr 20, 2015

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

@calavera
Copy link
Contributor Author

calavera commented Apr 20, 2015

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

LK4D4 commented Apr 20, 2015

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

@calavera
Copy link
Contributor Author

calavera commented Apr 20, 2015

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

LK4D4 commented Apr 20, 2015

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

LK4D4 commented Apr 20, 2015

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

calavera added 2 commits April 20, 2015 13:37
@calavera
Remove engine.Job references from builder.CmdBuild
9a2c009 
Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Remove engine.Job from builder.CmdBuildConfig.
ae40635 
Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the remove_engine_job_builder branch from 66dad95 to 701a868 Compare April 20, 2015 21:19
@calavera
Copy link
Contributor Author

calavera commented Apr 20, 2015

@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
LK4D4 reviewed Apr 20, 2015
View reviewed changes
api/server/server.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 20, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@calavera calavera Apr 20, 2015

Choose a reason for hiding this comment

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

agreed!

@calavera
Add flusher check to utils.WriteFlusher.
3b05005 
That way we can know when the stream has been flushed.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the remove_engine_job_builder branch from 701a868 to 3b05005 Compare April 20, 2015 21:25
@calavera
Copy link
Contributor Author

calavera commented Apr 20, 2015

green again 🎉

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 20, 2015

LGTM

1 similar comment
@jessfraz
Copy link
Contributor

jessfraz commented Apr 21, 2015

LGTM

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 jessfraz merged commit 82f75df into moby:master Apr 21, 2015
@calavera calavera deleted the remove_engine_job_builder branch April 21, 2015 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@calavera @LK4D4 @jessfraz @duglin @cpuguy83 @GordonTheTurtle