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.

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

adding split on commas in container_name_matches? definition #185

Merged
merged 2 commits into from Jul 10, 2014
Merged

adding split on commas in container_name_matches? definition #185

merged 2 commits into from Jul 10, 2014

Conversation

bplunkert
Copy link

This should fix issue #184

@bplunkert
Copy link
Author

To add a bit more detail: when a container links another container, the latter container's names will be updated to reflect the link.

i.e. containerA links containerB:xyz
names of containerB go from: "containerB" to "containerB,containerA/xyz"

Since comma is not an allowable character in a container name, it should be safe to split names by ',' and handle the first sub-string as the name of the container.

@bplunkert
Copy link
Author

I'm working on some tests for this (in test/cookbooks/docker_test/recipes/container_lwrp.rb and test/cookbooks/docker_test/files/default/tests/minitest/container_lwrp_test.rb) but running on so many suites takes a very long time, and it seems like there may be some pre-failing tests lurking about. If anyone can offer any time-saving pointers, that'd be helpful -- otherwise I'll keep trying and start filing issues for any failing tests I find along the way.

@bplunkert
Copy link
Author

I'm going to hold off on committing tests for this until tests are generally passing so as to avoid polluting them further. In the meantime, though, this fix is essential if you are linking containers and need to run chef more than once, so it may be worthwhile to merge it anyway?

@jf647
Copy link

jf647 commented Jul 9, 2014

On my system, the original container name isn't necessarily the first element of the CSV list of names:

b8425ae8eca9        cepl-postgresql:9.1   /bin/sh -c '/usr/bin   8 days ago          Up 8 days           0.0.0.0:5432->5432/tcp                             jira/db,postgresql,stash/db

That container was started with "--name postgresql", then later on I started two more named "stash" and "jira", both linked to "postgresql" as "db".

So the comparison would better be written as:

named.split(',').include?(new_resource.container_name)

Cheers

@bplunkert
Copy link
Author

@jf647 thanks! I hadn't considered that possibility, but your solution should definitely be more resilient. I've added a commit which implements the include? method.

@bflad
Copy link
Contributor

bflad commented Jul 10, 2014

This is an important fix. Thanks!

bflad added a commit that referenced this pull request Jul 10, 2014
adding split on commas in container_name_matches? definition
@bflad bflad merged commit 5493149 into sous-chefs:master Jul 10, 2014
@bplunkert bplunkert deleted the fix_container_name_matching branch July 21, 2014 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants