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

fix: config via environment #2725

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Aug 8, 2024

Fix the ability to set the configuration of testcontainers using an environment variable set by a caller before the use of testcontainers.

This removes the warning about disabling the reaper which is actually safe when done correctly.

Also optimise verbose check while there.

Fixes: #2701 #2636 #2609

@stevenh stevenh requested a review from a team as a code owner August 8, 2024 19:51
Copy link

netlify bot commented Aug 8, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit da4d087
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/66bf352377a26500080a928f
😎 Deploy Preview https://deploy-preview-2725--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

@stevenh we probably want to still log the message but not that early. I naively added it to the init method without considering the impact for users not being able to set the logger at the right time.

The current requirements for this would be:

  • allow users set their logger. At the moment, each container request supports passing a logger, otherwise, the default one will be used.
  • log the Ryuk-disabled message, using user's logger.

For a follow-up redesign (v1) we probably want to include the logger as part of the setup (see your comment in #2701 (comment))

Fix the ability to set the configuration of testcontainers using an
environment variable set by a caller before the use of testcontainers.

This removes the warning about disabling the reaper which is actually
safe when done correctly.

Optimise verbose check.

Improve Config struct and field documentation.

Fixes: #2701 #2636
@stevenh
Copy link
Collaborator Author

stevenh commented Aug 16, 2024

@stevenh we probably want to still log the message but not that early. I naively added it to the init method without considering the impact for users not being able to set the logger at the right time.

The current requirements for this would be:

  • allow users set their logger. At the moment, each container request supports passing a logger, otherwise, the default one will be used.
  • log the Ryuk-disabled message, using user's logger.

For a follow-up redesign (v1) we probably want to include the logger as part of the setup (see your comment in #2701 (comment))

I'd actually push back on that and suggest we don't log, here's why:

If someone has chosen to disable the reaper it will be a very conscious decision, and in doing so they are actively taking responsibility. While the user can't protect against leaked resources for a test timeout, that's the only case and we can document that and the extra noise makes it harder to see issues when they occur.

So to help unblock, until we can revisit for v1, I've pushed an update which improves the pkgsite docs, does that work?

@mdelapenya
Copy link
Member

If someone has chosen to disable the reaper it will be a very conscious decision

Indeed. Let's do as you propose here without the log. I know we have commented this internally, @kiview, and we want to log the message not using STDOUT but a user-defined logger. Let's keep this in mind for v1.

@mdelapenya
Copy link
Member

I've pushed an update which improves the pkgsite docs, does that work?

Not only the pkgsite docs but the docs site: https://deploy-preview-2725--testcontainers-go.netlify.app/features/configuration/ 👏

@mdelapenya mdelapenya self-assigned this Aug 16, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Aug 16, 2024
@mdelapenya mdelapenya merged commit c44b1b2 into testcontainers:main Aug 16, 2024
111 of 112 checks passed
@stevenh stevenh deleted the fix/config branch August 16, 2024 15:44
mdelapenya added a commit that referenced this pull request Aug 16, 2024
* main:
  fix: config via environment (#2725)
  fix(redpanda): race condition on port check (#2692)
  fix: logging restart (#2697)
  fix!: docker authentication setup (#2727)
  chore: improve error wrapping (#2720)
  chore: run make tests in verbose mode (#2734)
  chore(deps): bump github.com/docker/docker from 27.1.0+incompatible to 27.1.1+incompatible (#2733)
  fix(kafka): port race on start (#2696)
  docs: fix broken doc tags (#2732)
  fix: nginx request failures (#2723)
  fix(compose): container locking (#2722)
  fix(wait): log test timeout (#2716)
  chore: increase timeout values (#2719)
  chore: remove unused parameters (#2721)
  chore(mockserver): silence warning about internal port (#2730)
  feat(wait): skip internal host port check (#2691)
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]: Unable to disable Ryuk in 0.0.32
2 participants