-
Notifications
You must be signed in to change notification settings - Fork 18.9k
remove job from push #12505
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
remove job from push #12505
Conversation
51dab09 to
3bf7602
Compare
graph/push.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you dropping this code?
|
@duglin |
|
@HuKeping |
|
need rebase and the |
|
@dalanlan You can look how it solved in builder. There is detection if there was write to responsewriter, which means autowrite of |
71c2779 to
77e20ed
Compare
api/server/server.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @vieux as original author of such trick. Do we need it for import and push?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from what I understand is job.Stdout.Used() is replaced by output.Flushed() it's used to check if we already wrote on the socket, is we did, we cannot return an error (otherwise we will have duplicate write headers and such things)
Here I don't see job.Stdout.Used() on the old version so I don't think it's needed. I think Import will never write and return an error, either it returns an error or it writes, that why the check isn't needed.
I hope this is understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the problem. Things are quite similar there for pull and import.
Assume you wanna pull an non-existing image with current code, youll get an error http: multiple response.WriteHeader calls`.Seems that any attempt to write to output without check for Flused() would case such problems to my best knowledge.
https://github.com/docker/docker/blob/master/graph/pull.go#L46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty weird stuff :/ Import indeed writing somewhere.
|
due to recent changes you'll need to rebase. |
|
i`ll wait for the reply of vieux before doing the rebase and squash:) |
|
I think I'd be okay with |
77e20ed to
3393319
Compare
3393319 to
a68f8da
Compare
|
rebase needed |
Signed-off-by: Simei He <hesimei@zju.edu.cn> Signed-off-by: He Simei <hesimei@zju.edu.cn>
a68f8da to
d456401
Compare
|
Odd indeed. Fully functioned just now. Here is the log: |
|
the issue is being investigated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to move this to api/types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpuguy83 may I ask why? asking just because this is a struct needed only as a configuration paramenter to Push and not what we return through the api. Maybe what we write to OutStream should go to api/types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, I didn't realize that.
If it's only used by graph that's probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe it makes sense to structure what's written to the out stream in api/types as you said not needed atm..
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
part of #12151
remove job from push
Signed-off-by: Simei He hesimei@zju.edu.cn