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

Fix case-sensitivity bug for port numbers in command #524

Merged
merged 6 commits into from
Dec 17, 2017
Merged

Fix case-sensitivity bug for port numbers in command #524

merged 6 commits into from
Dec 17, 2017

Conversation

dnno
Copy link
Contributor

@dnno dnno commented Dec 16, 2017

This bug shows itself when a port number contains letters in the hexadecimal conversion.

This bug shows itself when a port number contains letters in the hexadecimal conversion.
@@ -29,7 +29,7 @@ public Boolean call() {

private void tryPort(Integer internalPort) {
String[][] commands = {
{"/bin/sh", "-c", format("cat /proc/net/tcp | awk '{print $2}' | grep :%x && echo %s", internalPort, SUCCESS_MARKER)},
{"/bin/sh", "-c", format("cat /proc/net/tcp | awk '{print $2}' | grep :%X && echo %s", internalPort, SUCCESS_MARKER)},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%x produces lower-case hexadecimal numbers, /proc/net/tcp has uppercase references. This only shows itself with ports that actually contain letters, of course. Hence the configuration change for the nginx instance to run on 8080, which is '1F90' in hex (as opposed to '1f90')...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoa! Nice one, thanks @dnno. We should probably change grep to -i to completely ignore the case in case of the case differences. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that sounds reasonable. Will change that!

@@ -3,31 +3,36 @@
import com.google.common.collect.ImmutableSet;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please fix the indentation in this file?
Also, IMO we can just use nc for this test to avoid creating a config for nginx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes, sorry, will fix the indentation.

I had a problem with netcat and the infinispan container before, it's not installed. I could imagine other containers not supporting it, too. In fact I thought this was the reason for the /proc/net/tcp solution here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok, I see 👍

@@ -0,0 +1,44 @@
server {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. please rename the file to something explicit, like nginx_on_8080.conf, to avoid the confusion if this config is reused in some other test
  2. server { listen 8080; } should be enough for that test and will better indicate the purpose of this config

@bsideup
Copy link
Member

bsideup commented Dec 17, 2017

@dnno please also mention your change in CHANGELOG.md :)

@bsideup bsideup merged commit ed64c1f into testcontainers:master Dec 17, 2017
@bsideup
Copy link
Member

bsideup commented Dec 17, 2017

Perfect! Thanks @dnno!

@dnno
Copy link
Contributor Author

dnno commented Dec 17, 2017

My pleasure! 😊

rnorth pushed a commit that referenced this pull request Dec 18, 2017
* Fix case-sensitivity bug for port numbers in command

This bug shows itself when a port number contains letters in the hexadecimal conversion.

* Make grep ignore casing

* Rename and cleanup Nginx test configuration file

* Update the changelog for PR #524

* Fix a typo in the changelog
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.

2 participants