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.

Sign up for GitHub

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

Jump to bottom

fix: incorrect parsing of exposedPorts in readiness check #2658

Merged
merged 5 commits into from
Aug 5, 2024
Merged

fix: incorrect parsing of exposedPorts in readiness check #2658

merged 5 commits into from
Aug 5, 2024

Conversation

robinvanderstraeten-klarrio
Copy link
Contributor

What does this PR do?

If an explicit port mapping is given in the definition of a container (through exposedPorts), the default readiness check fails. See the related issue below for an example. This PR fixes the issue by using the nat.ParsePortSpecs function instead of the nat.Port function when parsing the exposedPorts.

Why is it important?

Anyone who upgrades to v0.32.0 and has a port defined as for example 4443:4443/tcp will run into the below issue.

Related issues

How to test this PR

I still need to add a unit test for this, any advice on how to approach this?

@netlify Netlify
Copy link

netlify bot commented Jul 17, 2024 •
edited
Loading

✅ Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 352e5ac
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/66a8b0168acd130008b1f9fb
😎 Deploy Preview https://deploy-preview-2658--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya
Copy link
Collaborator

I still need to add a unit test for this, any advice on how to approach this?

you could extract the logic to a function and unit test it creating different port instances, covering all possible cases 🤔

@robinvanderstraeten-klarrio
Copy link
Contributor Author

Hi @mdelapenya , I've added some tests. Any more work that needs to be done for this PR?

stevenh
stevenh requested changes Jul 26, 2024
View reviewed changes
lifecycle.go Outdated Show resolved Hide resolved
lifecycle.go Outdated Show resolved Hide resolved
lifecycle.go Outdated Show resolved Hide resolved
lifecycle.go Outdated Show resolved Hide resolved
lifecycle_test.go Outdated Show resolved Hide resolved
lifecycle_test.go Outdated Show resolved Hide resolved
lifecycle_test.go Outdated Show resolved Hide resolved
lifecycle_test.go Outdated Show resolved Hide resolved
lifecycle_test.go Outdated Show resolved Hide resolved
lifecycle_test.go Outdated Show resolved Hide resolved
@robinvanderstraeten-klarrio
Copy link
Contributor Author

Thanks for the suggestions @stevenh , applied them here: 5fe2f77

stevenh
stevenh requested changes Jul 27, 2024
View reviewed changes
Copy link
Collaborator

@stevenh stevenh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for applying the previous suggestions, few more came out of that, if you would be so kind :)

lifecycle.go Outdated Show resolved Hide resolved
lifecycle.go Outdated Show resolved Hide resolved
@robinvanderstraeten-klarrio
Copy link
Contributor Author

Sure @stevenh , applied them here: 42344f6

stevenh
stevenh approved these changes Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@mdelapenya mdelapenya self-assigned this Aug 5, 2024
mdelapenya
mdelapenya approved these changes Aug 5, 2024
View reviewed changes
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdelapenya mdelapenya added the bug An issue with the library label Aug 5, 2024
@mdelapenya
Copy link
Collaborator

Thanks @stevenh for driving the review, much appreciated!

@mdelapenya mdelapenya merged commit 9e8d4e7 into testcontainers:main Aug 5, 2024
110 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Aug 6, 2024
@mdelapenya
Merge branch 'main' into host-port-binding-ip-family
553e925 
* main:
  chore: print Docker Info labels in banner (testcontainers#2681)
  fix: incorrect parsing of exposedPorts in readiness check (testcontainers#2658)
  feat: add grafana-lgtm module (testcontainers#2660)
  Added valkey module (testcontainers#2639)
  fix: container.Endpoint and wait.FortHTTP to use lowest internal port (testcontainers#2641)
mdelapenya added a commit that referenced this pull request Aug 7, 2024
@mdelapenya
Merge branch 'main' into v1
36fff00 
* main:
  fix: missing image build errors (#2651)
  test: fix image label test (#2689)
  chore(deps): bump github.com/docker/docker from 27.0.3+incompatible to 27.1.0+incompatible (#2682)
  chore: print Docker Info labels in banner (#2681)
  fix: incorrect parsing of exposedPorts in readiness check (#2658)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenh stevenh stevenh approved these changes

@mdelapenya mdelapenya mdelapenya approved these changes

Assignees

@mdelapenya mdelapenya

Labels
bug An issue with the library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Explicitely mapped ports break default readiness hook
3 participants
@robinvanderstraeten-klarrio @mdelapenya @stevenh