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

GenericContainer: in case of error: return a reference to the failed container #2082

Merged
merged 1 commit into from
Jan 9, 2024
Merged

GenericContainer: in case of error: return a reference to the failed container #2082

merged 1 commit into from
Jan 9, 2024

Conversation

JordanP
Copy link
Contributor

@JordanP JordanP commented Jan 9, 2024

What does this PR do?

GenericContainer function: in case of error: return a reference to the failed container. This way, the caller has an opportunity to clean things up and call Destroy on the failed container.

Why is it important?

If a WaitStrategy fails (timeouts for instance, context canceled, etc.), we may still want to return a reference to the running container so that the caller can Destroy() it and not leave half-healthy containers around.

…container

This way, the caller has an opportunity to clean things up and call
Destroy on the failed container.
@JordanP JordanP requested a review from a team as a code owner January 9, 2024 12:16
Copy link

netlify bot commented Jan 9, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit f150875
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/659d3929fe84b4000830c802
😎 Deploy Preview https://deploy-preview-2082--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
Member

mdelapenya commented Jan 9, 2024

Hi @JordanP thanks for submitting this enhancement. I see the value in it although I'd like to confirm with you that this

not leave half-healthy containers around.

will happen if Ryuk is disabled, right? If Ryuk is enabled then there is no risk of leaking containers.

@mdelapenya mdelapenya self-assigned this Jan 9, 2024
@mdelapenya mdelapenya added the enhancement New feature or request label Jan 9, 2024
@JordanP
Copy link
Contributor Author

JordanP commented Jan 9, 2024

will happen if Ryuk is disabled, right?

Yes, that's correct.

More generally, I would say it's fair to let the caller (that tried to create the container) deal with the failed container, if it wants to. But, to give the caller a chance to do that, it needs a reference to the container.

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 @JordanP for changing this. Many times, tiny things are the hardest to discover. Thank you

@mdelapenya mdelapenya merged commit f051b0c into testcontainers:main Jan 9, 2024
116 checks passed
@JordanP JordanP deleted the jordan-return-container-ref branch January 9, 2024 15:22
mdelapenya added a commit to jespino/testcontainers-go that referenced this pull request Jan 9, 2024
* main:
  chore: move internal/testcontainersdocker package's files to internal/core (testcontainers#2083)
  GenericContainer: in case of error: return a reference to the failed container (testcontainers#2082)
  [breaking] Add err chan to log producer and don't panic on error (testcontainers#1971)
  chore: enrich HTTP headers to the Docker daemon with the project path (testcontainers#2080)
  fix: align codeql versions in GH workflow (testcontainers#2081)
  chore(deps): bump go.mongodb.org/mongo-driver in /modules/mongodb (testcontainers#2065)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.11 to 3.23.12 (testcontainers#2068)
  fix(modules.gcloud): pass as ptr to allow request customization (testcontainers#1972)
  chore(deps): bump github.com/twmb/franz-go in /modules/redpanda (testcontainers#2072)
  chore(deps): bump k8s.io/api, k8s.io/apimachinery, k8s.io/client-go from 0.28.4 to 0.29.0 in /modules/k3s (testcontainers#2078)
  chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (testcontainers#2066)
  chore(deps): bump github.com/google/uuid from 1.4.0 to 1.5.0 (testcontainers#2077)
  bump google.golang.org/api from 0.153.0 to 0.154.0, cloud.google.com/go/spanner from 1.53.1 to 1.54.0, bump google.golang.org/grpc from 1.59.0 to 1.60.1 in /modules/gcloud (testcontainers#2076)
  chore(deps): bump github.com/aws/aws-sdk-go-v2 from 1.23.5 to 1.24.0 (credentials from 1.16.9 to 1.16.13, service/s3 from 1.47.1 to 1.47.7)  in /modules/localstack (testcontainers#2075)
  chore(deps): bump github/codeql-action from 2 to 3 (testcontainers#2056)
  chore(deps): bump test-summary/action from 2.1 to 2.2 (testcontainers#2058)
  chore(deps): bump actions/setup-go from 4 to 5 (testcontainers#2057)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants