Skip to content

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

Merged
bsideup merged 6 commits into
testcontainers:masterfrom
dnno:master
Dec 17, 2017
Merged

Fix case-sensitivity bug for port numbers in command#524
bsideup merged 6 commits into
testcontainers:masterfrom
dnno:master

Conversation

@dnno
Copy link
Copy Markdown
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.
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 👍

Comment thread core/src/test/resources/nginx.conf Outdated
@@ -0,0 +1,44 @@
server {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

bsideup commented Dec 17, 2017

Perfect! Thanks @dnno!

@dnno
Copy link
Copy Markdown
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