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

ISSUE-1327 start ambassador only if mapping configured #1328

Merged
merged 1 commit into from
Mar 21, 2019

Conversation

ftardif
Copy link
Contributor

@ftardif ftardif commented Mar 19, 2019

fix #1121
edit: that was a typo, this PR fix: #1327

@bsideup bsideup added this to the next milestone Mar 20, 2019
@bsideup
Copy link
Member

bsideup commented Mar 20, 2019

Hi @ftardif,

Thanks a lot for fixing it! Could you please add a simple test to verify the fix?

@ftardif
Copy link
Contributor Author

ftardif commented Mar 20, 2019

Do you have a strategy to test these kind of behaviours? I cannot do a unit test by mocking the ambassador generic container because it is instantiated within the class. And I am not too sure how I could do an integration test to verify that socat container is not launched. Any recommendation?

@bsideup
Copy link
Member

bsideup commented Mar 20, 2019

@ftardif AFAIR it will fail if there are no ports exposed, right? Can we test it?

@ftardif
Copy link
Contributor Author

ftardif commented Mar 20, 2019

No the idea is to work, but without the socat container, only the ryuk and actual containers unwind from the compose file. When the containers already have host port exposed, we can use these for waiting strategies, hence the basic socat check becomes un relevant.

@ftardif
Copy link
Contributor Author

ftardif commented Mar 21, 2019

this PR is meant to fix : #1327

@bsideup bsideup changed the title ISSUE-1121 start ambassador only if mapping configured ISSUE-1327 start ambassador only if mapping configured Mar 21, 2019
@bsideup
Copy link
Member

bsideup commented Mar 21, 2019

reported #1331, will merge this one without a test

@bsideup bsideup merged commit 31b3610 into testcontainers:master Mar 21, 2019
@trajano
Copy link

trajano commented Mar 21, 2019

So is #1121 fixed for this is it still a typo?

@bsideup
Copy link
Member

bsideup commented Mar 21, 2019

@ftardif thanks for your contribution! 👍

@trajano I think #1121 is a different problem

@trajano
Copy link

trajano commented Mar 21, 2019

OK because it was marked as closed due to this PR.

@rnorth
Copy link
Member

rnorth commented Mar 22, 2019

Releasing this in 1.11.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants