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

Allow overriding Docker api gem options #509

Closed
wants to merge 1 commit into from
Closed

Allow overriding Docker api gem options #509

wants to merge 1 commit into from

Conversation

carlossg
Copy link
Member

Do not clean the Docker.options, as they may be already set

Specifically to allow the workaround from upserve/docker-api#202 for
Excon::Errors::SocketError: end of file reached (EOFError)

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@branan
Copy link
Contributor

branan commented Oct 27, 2014

OK to test

@branan
Copy link
Contributor

branan commented Oct 27, 2014

It would be awesome if you could add a test for the "docker options already set" case

@colinPL
Copy link
Contributor

colinPL commented Oct 28, 2014

ok to test

@puppetlabs-jenkins
Copy link
Contributor

🔴 Test failed.
Refer to this link for build results: http://jenkins-beaker.delivery.puppetlabs.net/job/Beaker%20Combined%20Smoketest%20(ec2)/220/

@branan
Copy link
Contributor

branan commented Oct 29, 2014

@carlossg The spec tests are failing with your change. Your code assumes that Docker.options is always initialized, but that is not necessarily the case. You should ensure that if Docker.options is not set that it gets a value (probably {}) before attempting to assign to its members.

@carlossg
Copy link
Member Author

@branan on my way back from devops days, didn't forget about it ;) will send fix and test asap

Allow overriding Docker api gem options
5e56663 
Do not clean the Docker.options, as they may be already set
@carlossg
Copy link
Member Author

carlossg commented Nov 2, 2014

Fixed the nil case and added new tests

@puppetlabs-jenkins
Copy link
Contributor

💚 Test passed.
Refer to this link for build results: http://jenkins-beaker.delivery.puppetlabs.net/job/Beaker%20Combined%20Smoketest%20(ec2)/233/

@electrical
Copy link

LGTM

@anodelman
Copy link
Contributor

I'll update so that this meets contributor documentation requirements, then it can be merged.

anodelman pushed a commit to anodelman/beaker that referenced this pull request Nov 3, 2014
(QENG-1450) when docker options are already set they should...
fd98e13 
... not be overridden

- as reported in voxpupuli#509
- repackaged PR to meet contributor guidelines
@anodelman anodelman mentioned this pull request Nov 3, 2014
Merged
@anodelman
Copy link
Contributor

Closing in favor of #520, just pulling in line with contributor doc.

@anodelman anodelman closed this Nov 3, 2014
@carlossg carlossg deleted the docker-api branch November 9, 2014 10:44
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@carlossg @puppetlabs-jenkins @branan @colinPL @electrical @anodelman