Skip to content

remove execCreate&execStart from job #12472

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
Closed
wonderflow wants to merge 1 commit into from
Closed

remove execCreate&execStart from job #12472

wonderflow wants to merge 1 commit into from
+51 −84

Conversation

@wonderflow
Copy link
Contributor

@wonderflow wonderflow commented Apr 17, 2015

Relating with #12151

Also remove the function ExecConfigFromJob .

Signed-off-by: Sun Jianbo wonderflow@zju.edu.cn

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 17, 2015

@wonderflow Thanks for the work here.
One of the reasons we want to remove job is because of how horribly bad env is.

Can we move this from using env to use type safe functions/structs?

@wonderflow
Copy link
Contributor Author

wonderflow commented Apr 17, 2015

@cpuguy83
We use env to decode the http messages and then transfer it to daemon.containerExecCreate.

So do you mean to rewrite a new way to decode http messages?

@wonderflow
Copy link
Contributor Author

wonderflow commented Apr 17, 2015

@cpuguy83 I have known what you mean. Thanks. Let me change it.

@wonderflow wonderflow force-pushed the 12151_remove_engine branch from b66a33f to 8d7999d Compare April 17, 2015 13:59
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from 8d7999d to 34afabb Compare April 17, 2015 15:19
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from 34afabb to fe43c5b Compare April 17, 2015 15:56
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from fe43c5b to c1ca84a Compare April 17, 2015 16:05
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from c1ca84a to 48997c0 Compare April 17, 2015 16:16
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from 48997c0 to 846300f Compare April 17, 2015 16:23
@icecrime icecrime mentioned this pull request Apr 17, 2015
Closed
41 tasks
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 18, 2015

@wonderflow a lot of tests failing :)

@wonderflow
Copy link
Contributor Author

wonderflow commented Apr 18, 2015

@LK4D4 thanks, I will fix it soon.

@wonderflow wonderflow force-pushed the 12151_remove_engine branch from 846300f to 8afeb2e Compare April 18, 2015 06:30
@wonderflow
Copy link
Contributor Author

wonderflow commented Apr 20, 2015

@duglin squashed.

cpuguy83
cpuguy83 reviewed Apr 20, 2015
View reviewed changes
daemon/exec.go Outdated
Copy link
Member

@cpuguy83 cpuguy83 Apr 20, 2015

Choose a reason for hiding this comment

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

Let's switch these from Stdout and Stderr, to stdout and stderr

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 20, 2015

Also needs another rebase already, sorry.

@wonderflow wonderflow force-pushed the 12151_remove_engine branch from 71d2d48 to a3a8af6 Compare April 20, 2015 15:08
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from a3a8af6 to 47abb0d Compare April 20, 2015 15:26
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from 47abb0d to 5e6d7e8 Compare April 20, 2015 15:50
@wonderflow
Copy link
Contributor Author

wonderflow commented Apr 20, 2015

@cpuguy83 I've done it. Thanks!

cpuguy83
cpuguy83 reviewed Apr 20, 2015
View reviewed changes
api/server/server.go Outdated
Copy link
Member

@cpuguy83 cpuguy83 Apr 20, 2015

Choose a reason for hiding this comment

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

This might be a nit, but I'd prefer to not decode to ExecConfig here, instead add something in api/types for this, especially since we're really just getting Detach and Tty here.

@duglin @LK4D4

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.

Okay, can be temp type here.

@wonderflow wonderflow force-pushed the 12151_remove_engine branch 2 times, most recently from f836ab3 to 903bc55 Compare April 21, 2015 02:11
@wonderflow
Copy link
Contributor Author

wonderflow commented Apr 21, 2015

@cpuguy83 @LK4D4 Finished, please check. Thanks!

LK4D4
LK4D4 reviewed Apr 21, 2015
View reviewed changes
api/server/server.go Outdated
Copy link
Contributor

@LK4D4 LK4D4 Apr 21, 2015

Choose a reason for hiding this comment

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

why you creating this at all if you later switch it to outStream? Also I'm pretty sure that using os.Stderr directly in daemon is not right thing.

Copy link
Contributor Author

@wonderflow wonderflow Apr 22, 2015

Choose a reason for hiding this comment

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

This is left by the old code, I'll remove it then. @LK4D4

@wonderflow wonderflow force-pushed the 12151_remove_engine branch 2 times, most recently from 7ee736b to 7688eaf Compare April 22, 2015 01:31
@wonderflow
remove execCreate & execStart from job
9aaaa90 
Also removed the function ExecConfigFromJob

Signed-off-by: Sun Jianbo <wonderflow@zju.edu.cn>
@wonderflow wonderflow force-pushed the 12151_remove_engine branch from 7688eaf to 9aaaa90 Compare April 22, 2015 01:35
calavera
calavera reviewed Apr 22, 2015
View reviewed changes
api/server/server.go
Copy link
Contributor

@calavera calavera Apr 22, 2015

Choose a reason for hiding this comment

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

I think name := vars["name] is more idiomatic

@calavera
Copy link
Contributor

calavera commented Apr 22, 2015

Two minor code formatting issue, that are not blocking at all. Besides that, it looks good to me so far.

@LK4D4 LK4D4 mentioned this pull request Apr 22, 2015
Merged
@wonderflow wonderflow deleted the 12151_remove_engine branch April 22, 2015 23:12
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.

8 participants

@wonderflow @cpuguy83 @LK4D4 @duglin @calavera @thaJeztah @icecrime @GordonTheTurtle