Skip to content

Conversation

@dalanlan
Copy link
Contributor

part of #12151
remove job from push
Signed-off-by: Simei He hesimei@zju.edu.cn

graph/push.go Outdated
Copy link
Contributor

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?

@dalanlan
Copy link
Contributor Author

@duglin
Seems that there is an error http: multiple response.WriteHeader calls due to simultaneous call of return fmt.Errorf & imagePushConfig.OutStream.Write, or whatever sth like that. The same problem occurs in pull case exactly.
But i havent figured out how to fix it :(

@dalanlan
Copy link
Contributor Author

@HuKeping
:)

@HuKeping
Copy link
Contributor

need rebase and the OutStream has not been init?

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 21, 2015

@dalanlan You can look how it solved in builder. There is detection if there was write to responsewriter, which means autowrite of 200 OK.

@dalanlan dalanlan force-pushed the remove_job_from_push branch 4 times, most recently from 71c2779 to 77e20ed Compare April 21, 2015 06:55
@dalanlan
Copy link
Contributor Author

@LK4D4 Fixed. Appreciated it.
@HuKeping Thx :)

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

@duglin
Copy link
Contributor

duglin commented Apr 21, 2015

due to recent changes you'll need to rebase.
also, while doing that, please squash it down to one commit

@dalanlan
Copy link
Contributor Author

i`ll wait for the reply of vieux before doing the rebase and squash:)

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 22, 2015

I think I'd be okay with Used if tests will pass.

@dalanlan
Copy link
Contributor Author

@LK4D4 @duglin
Fixed:)

@duglin
Copy link
Contributor

duglin commented Apr 23, 2015

rebase needed

Signed-off-by: Simei He <hesimei@zju.edu.cn>

Signed-off-by: He Simei <hesimei@zju.edu.cn>
@dalanlan
Copy link
Contributor Author

Odd indeed. Fully functioned just now.
SeemsTestRenameStoppedContainer has nothing to do with my code.

Here is the log:
FAIL: docker_cli_rename_test.go:10: DockerSuite.TestRenameStoppedContainer docker_cli_rename_test.go:31: c.Fatalf(out, err) ... Error: Error response from daemon: Error when allocating new name: Conflict. The name "new_name" is already in use by container b594bddc0c2f. You have to delete (or rename) that container to be able to reuse that name. time="2015-04-23T13:37:06Z" level=fatal msg="Error: failed to rename container named first_name" %!(EXTRA *exec.ExitError=exit status 1)

@duglin
Copy link
Contributor

duglin commented Apr 23, 2015

the issue is being investigated

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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..

@cpuguy83
Copy link
Member

LGTM

2 similar comments
@duglin
Copy link
Contributor

duglin commented Apr 23, 2015

LGTM

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 23, 2015

LGTM

LK4D4 added a commit that referenced this pull request Apr 23, 2015
@LK4D4 LK4D4 merged commit 2351b87 into moby:master Apr 23, 2015
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