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: container.Endpoint and wait.FortHTTP to use lowest internal port #2641

Merged
merged 1 commit into from
Jul 18, 2024
Merged

fix: container.Endpoint and wait.FortHTTP to use lowest internal port #2641

merged 1 commit into from
Jul 18, 2024

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Jul 12, 2024 •
edited
Loading

Ensure Container.Endpoint returns the lowest numbered aka first port when there is more than one exposed.

Ensure that wait.ForHTTP uses the lowest numbered aka first port when there is more than one exposed.

Prior to this a random port would be returned leading to unpredictable
results.

Fixed: #2640

@stevenh stevenh requested a review from a team as a code owner July 12, 2024 19:57
@netlify Netlify
Copy link

netlify bot commented Jul 12, 2024 •
edited
Loading

✅ Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit d0b3354
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/669852a27a6181000829216a
😎 Deploy Preview https://deploy-preview-2641--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.

@stevenh stevenh marked this pull request as draft July 12, 2024 20:35
@stevenh stevenh force-pushed the fix/endpoint branch 3 times, most recently from f8a09cb to b573705 Compare July 12, 2024 22:54
@stevenh stevenh changed the title fix: endpoint not returning the first port fix: endpoint and http wait use of wrong port Jul 12, 2024
@stevenh stevenh marked this pull request as ready for review July 12, 2024 23:58
@mdelapenya
Copy link
Member

Hi @stevenh do you think we should mark this as a breaking change, as we are changing the docs from first to lowest?

Other than that, LGTM

@mdelapenya mdelapenya self-assigned this Jul 17, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Jul 17, 2024
@stevenh
Copy link
Collaborator Author

stevenh commented Jul 17, 2024

Hi @stevenh do you think we should mark this as a breaking change, as we are changing the docs from first to lowest?

Other than that, LGTM

I don't think so as I believe that original intent of first was the first port numerically however this wasn't totally clear, hence the change in language disambiguate.

@mdelapenya
Copy link
Member

Wdyt if we change the title to fix: use the lowest numbered (aka first) port when there is more than one exposed so users read it more clear in the release notes?

@stevenh stevenh changed the title fix: endpoint and http wait use of wrong port fix: conainer.Endpoint and wait.FortHTTP to use lowest numbered internal port Jul 17, 2024
@stevenh stevenh changed the title fix: conainer.Endpoint and wait.FortHTTP to use lowest numbered internal port fix: container.Endpoint and wait.FortHTTP to use lowest internal port Jul 17, 2024
@stevenh
Copy link
Collaborator Author

stevenh commented Jul 17, 2024

Wdyt if we change the title to fix: use the lowest numbered (aka first) port when there is more than one exposed so users read it more clear in the release notes?

Updated the title to something similar, which I hope is a little bit more concise, let me know what you think?

@stevenh
fix: container.Endpoint and wait.FortHTTP to use lowest internal port
d0b3354 
Ensure Container.Endpoint returns the lowest numbered aka first port
there is more than one exposed.

Ensure that wait.ForHTTP uses the lowest numbered aka first port
when there is more than one exposed.

Prior to this a random port would be returned leading to unpredictable
results.

Clarify which port is uses by using lowest vs first, which could
otherwise be interpreted as the first one specified.

Fixed: #2640
mdelapenya
mdelapenya approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member

@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 merged commit 361d21d into testcontainers:main Jul 18, 2024
108 checks passed
@stevenh stevenh deleted the fix/endpoint branch July 18, 2024 10:49
mdelapenya added a commit that referenced this pull request Aug 5, 2024
@mdelapenya
Merge branch 'main' into v1
094d0b1 
* main:
  feat: add grafana-lgtm module (#2660)
  Added valkey module (#2639)
  fix: container.Endpoint and wait.FortHTTP to use lowest internal port (#2641)
  chore: test cleanups (#2657)
  docs: fix compilation of examples (#2656)
  feat: add custom container registry substitutor (#2647)
  fix: couchbase containers intermittently hang on startup (#2650)
  chore(deps): bump Ryuk to 0.8.1 (#2648)
  fix: retry on label error (#2644)
  perf: optimise docker authentication config lookup (#2646)
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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]: Endpoint returns the wrong port randomly
2 participants
@stevenh @mdelapenya