Skip to content
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

Input configuration should not be altered as it breaks unix socket support #457

Merged
merged 2 commits into from
Feb 12, 2016
Merged

Input configuration should not be altered as it breaks unix socket support #457

merged 2 commits into from
Feb 12, 2016

Conversation

mbaechler
Copy link
Contributor

… socket support

@mbaechler
Copy link
Contributor Author

fixes #456

@mbaechler mbaechler mentioned this pull request Feb 11, 2016
Closed
@mbaechler
Copy link
Contributor Author

could someone have a look at this simple patch ?

@KostyaSha
Copy link
Member

I'm not an expert of factory pattern, please wait for @marcuslinke reply.

@marcuslinke
Copy link
Contributor

@mbaechler Your patch seems fine but I needs to check whether the same issue affects the master branch first. Will merge ASAP. Thanks!

@mbaechler
Copy link
Contributor Author

thanks !
If you think I can add some tests somewhere to trigger the bug, please tell me.

@KostyaSha
Copy link
Member

Initialise class twice and compare that url not modified?

@mbaechler
Copy link
Contributor Author

It would test the bug's cause, but this behavior is not a problem by itself, it's the context that transform this side-effect into a bug.
If we were to write this side-effect free test, we would had better time make the configuration immutable instead.

@marcuslinke
Copy link
Contributor

@mbaechler Right. Please remove setUri from DockerClientConfig to make it immutable.

@mbaechler
Copy link
Contributor Author

ok, fixup push

@marcuslinke marcuslinke mentioned this pull request Feb 12, 2016
Closed
@marcuslinke
Copy link
Contributor

@mbaechler Thanks. Lets wait for travis before merge. Seems it has some (performance) issue today.

marcuslinke added a commit that referenced this pull request Feb 12, 2016
@marcuslinke
Merge pull request #457 from mbaechler/issue-456
4410848 
Issue-456 input configuration should not be altered as it breaks unix…
@marcuslinke marcuslinke merged commit 4410848 into docker-java:2.x Feb 12, 2016
This was referenced Feb 13, 2016
Closed
Merged
@marcuslinke marcuslinke changed the title Issue-456 input configuration should not be altered as it breaks unix… Input configuration should not be altered as it breaks unix socket support Feb 13, 2016
@marcuslinke marcuslinke mentioned this pull request Feb 15, 2016
Closed
@mbaechler mbaechler mentioned this pull request Feb 16, 2016
Merged
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
type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mbaechler @KostyaSha @marcuslinke