-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix internal port check when other ports are opened as well on the target container #2363
Conversation
Hi @codablock, Could you please add tests for it? |
@bsideup I added 3 commits to this PR.
|
core/src/test/resources/internal-port-check-dockerfile/Dockerfile-bash
Outdated
Show resolved
Hide resolved
core/src/test/resources/internal-port-check-dockerfile/Dockerfile-tcp
Outdated
Show resolved
Hide resolved
.../java/org/testcontainers/containers/wait/internal/InternalCommandPortListeningCheckTest.java
Show resolved
Hide resolved
Handled review comments. I also noticed that I forgot to commit an important change in |
Test failures are not reproducible locally. And it's unfortunately the new test that fails, so it's likely related. It's also strange that they succeed on circleci. Any ideas? |
@codablock I just triggered a re-run, let's see how it goes |
Tests failed again. But I think I found it: The |
The port check tests are green now. Other test failures on Azure Pipelines are unrelated ( |
@codablock it seems there was an issue with the disk space on Azure, I've submitted #2376 to fix it. Let's update your PR and do another run once that PR is merged to ensure that this change does not bring any side effects 👍 |
This one is never used and probably causes some garbage on the docker side.
… check When at least one process is listening for a port that is significantly higher then the port we're checking, this check will fail as the checked port will contain leading zeros in the output of "cat /proc/net/tcp". This commit fixes this by changing the grep to ":0*XXX".
Otherwise unnecessary shell expansion is tried
…ssible checks This commit refactors InternalCommandPortListeningCheckTest to work with 3 different nginx based containers, each being able to only succeed one of the 3 tests performed in InternalCommandPortListeningCheck. Each test is now executed agains all 3 containers to ensure that all tests work as expected.
This test should fail without the leading zeros fix found a few commits before this one.
It's not only port 8080 anymore, so better not have it in the name.
@codablock merged, thanks! 🎉 |
This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉 Thanks for the contribution! |
…rget container (testcontainers#2363) * Remove redundant execCreateCmd call This one is never used and probably causes some garbage on the docker side. * Allow leading zeros in /proc/net/tcp* when doing internal port listen check When at least one process is listening for a port that is significantly higher then the port we're checking, this check will fail as the checked port will contain leading zeros in the output of "cat /proc/net/tcp". This commit fixes this by changing the grep to ":0*XXX". * Surround grep parameter with ' Otherwise unnecessary shell expansion is tried * Refactor/Rewrite InternalCommandPortListeningCheckTest to test all possible checks This commit refactors InternalCommandPortListeningCheckTest to work with 3 different nginx based containers, each being able to only succeed one of the 3 tests performed in InternalCommandPortListeningCheck. Each test is now executed agains all 3 containers to ensure that all tests work as expected. * Add test for low+high ports This test should fail without the leading zeros fix found a few commits before this one. * Check for bash existence instead of only mentioning it in a comment * Rename nginx_on_8080.conf to nginx.conf It's not only port 8080 anymore, so better not have it in the name. * Make container a @rule again by moving initialization into contructor * Wait a few seconds for InternalCommandPortListeningCheck to pass
This PR contains 2 commits. The first one is a trivial cleanup of a redundant execCreateCmd call. The second commit contains the actual fix. Description from the second commit: