X Tutup
Skip to content

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

Merged
marcuslinke merged 2 commits intodocker-java:2.xfrom
mbaechler:issue-456
Feb 12, 2016
Merged

Input configuration should not be altered as it breaks unix socket support#457
marcuslinke merged 2 commits intodocker-java:2.xfrom
mbaechler:issue-456

Conversation

@mbaechler
Copy link
Copy Markdown
Contributor

@mbaechler mbaechler commented Feb 11, 2016

… socket support

@mbaechler
Copy link
Copy Markdown
Contributor Author

mbaechler commented Feb 11, 2016

fixes #456

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

mbaechler commented Feb 12, 2016

could someone have a look at this simple patch ?

@KostyaSha
Copy link
Copy Markdown
Member

KostyaSha commented Feb 12, 2016

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

@marcuslinke
Copy link
Copy Markdown
Contributor

marcuslinke commented Feb 12, 2016

@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
Copy Markdown
Contributor Author

mbaechler commented Feb 12, 2016

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

@KostyaSha
Copy link
Copy Markdown
Member

KostyaSha commented Feb 12, 2016

Initialise class twice and compare that url not modified?

@mbaechler
Copy link
Copy Markdown
Contributor Author

mbaechler commented Feb 12, 2016

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

marcuslinke commented Feb 12, 2016

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

@mbaechler
Copy link
Copy Markdown
Contributor Author

mbaechler commented Feb 12, 2016

ok, fixup push

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

marcuslinke commented Feb 12, 2016

@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
X Tutup