-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Don't set container name to node hostname #4
Conversation
Can one of the admins verify this patch? |
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 can verify that the issue and that applying this patch fixes it.
@jovrum Please add a test so we can ensure we don't break this again in the future. |
An interesting point could be to enable reusing containers with |
@ekohl, FWIW, you can set |
cc915c1
to
dcb70a2
Compare
@mchllweeks, I amended the commit to fix the tests, but ideally there should be an explicit test of the absence of the 'name' parameter (except for the 'docker_container_name' test), with a comment explaining why. Any rspec veterans know of a clean way to do that here? |
would adding |
@ekohl, where? |
I tried my suggestion but it doesn't work so I'm out of ideas. |
@jovrum is hash_excluding what you want? https://github.com/rspec/rspec-mocks listed under "Argument Matchers" in the README |
last comment still current |
This prevents running beaker concurrently with other beaker nodesets containing nodes with the same hostname.
dcb70a2
to
d268239
Compare
@ferglor, thanks! Updated the test to use hash_excluding. |
@puppetlabs-jenkins retest this please |
As pointed out here, PR #3 introduced a regression by setting the container name to the node hostname by default. This prevents running beaker concurrently with other beaker nodesets containing nodes with the same hostname.
This patch simply leaves that option blank by default (pre-PR3 behavior).