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

Handle error properly during port forwarding initialization. #2550

Conversation

cedric-appdirect
Copy link
Contributor

What does this PR do?

This PR does handle properly the error case while creating a ssh connection needed for port forwarding.

Why is it important?

Without this, if the ssh connection could not be established for some reason, it would leave the wait on the channel lock forever and ignore context timeout. This would make test hang for a long time on my Mac.

Related issues

How to test this PR

Running the port_forwarding tests on Mac should now error early with a proper error message.

Follow-ups

Need to adjust the port_forwarding tests depending on the platform they are run on.

@cedric-appdirect cedric-appdirect requested a review from a team as a code owner May 22, 2024 19:25
Copy link

netlify bot commented May 22, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 64157a5
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/665f895e51c22000084b77bf
😎 Deploy Preview https://deploy-preview-2550--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 mdelapenya added the bug An issue with the library label Jun 4, 2024
@mdelapenya
Copy link
Member

Hi @cedric-appdirect thanks for filling the issue and submitting this patch. I'm currently running the CI to verify everything works as expected. From the technical standpoint, it seems correct to handle the eventual error with a channel of error. BTW, could you share more on the error you experimented?

Sorry if took longer than expected, I was n PTO for a week and half so going back to business atm.

Thanks!

@cedric-appdirect
Copy link
Contributor Author

No worry. I kind of vaguely remember the issue is related to using testcontainer-go on MacOS where the network behave a bit differently on what you would expect. I ended up with a code that worked on Linux, but wouldn't on MacOS. I could try looking at it again to find exactly the scenario if you need it. I tried to figure out a way to trigger the error path on Linux, but couldn't find a way.

@cedric-appdirect
Copy link
Contributor Author

Do you know why test-modules (1.21.x, ubuntu-latest, couchbase) / modules/couchbase/ubuntu-latest/1.21.x is failing?

@mdelapenya
Copy link
Member

Do you know why test-modules (1.21.x, ubuntu-latest, couchbase) / modules/couchbase/ubuntu-latest/1.21.x is failing?

eventual flakiness. Need to check it more in depth. Meanwhile, I'm retying that build

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 f4ce8c5 into testcontainers:main Jun 7, 2024
104 checks passed
mdelapenya added a commit that referenced this pull request Jun 10, 2024
* main:
  Handle error properly during port forwarding initialization. (#2550)
mdelapenya added a commit to bearrito/testcontainers-go that referenced this pull request Jun 11, 2024
* main: (48 commits)
  Fix race condition when looking up reaper (ryuk) container (testcontainers#2508)
  chore: bring golangci-lint back (testcontainers#2571)
  docs(compose): Fix typo docker compose docs (testcontainers#2565)
  Handle error properly during port forwarding initialization. (testcontainers#2550)
  chore: pin vearch version (testcontainers#2568)
  feat: add vearch module (testcontainers#2560)
  chore: run tests against latest Docker engine, nightly (testcontainers#2566)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.0.7 (testcontainers#2562)
  Fix network accessor for port-forwarding feature (testcontainers#2551)
  --- (testcontainers#2549)
  fix: update search bar eval in mkdocs (testcontainers#2547)
  docs: improve contributing docs for code snippets (testcontainers#2546)
  chore: use a virtualenv for working with the docs site (testcontainers#2545)
  docs: document test session semantics (testcontainers#2544)
  feat(ryuk): allow to configure ryuk timeouts using env variables (testcontainers#2541)
  docs: fix CircleCI docs (testcontainers#2539)
  fix: add import to module generation (testcontainers#2537)
  chore: prepare for next minor development cycle (0.32.0)
  chore: use new version (v0.31.0) in modules and examples
  feat(mongodb): add replica set support via opts (testcontainers#2469)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: context.Context is ignored while setting up port forwarding
2 participants