bug: fix leaking logs

[KenxinKun]

What does this PR do?

Adds a new non-breaking option func (hp *HostPortStrategy) WithLogger(logger Logging) *HostPortStrategy.

Why is it important?

The linked issue identified leaking logs when waiting for exposed/listening ports. The proposed implementation preserves the current behaviour when not providing the option.

Related issues

• Closes [Bug]: Leaking logs despite configuring loggers #2909

How to test this PR

New unit tests are included using mock implementations of the logger interface.

Follow-ups

Consider if the proposed implementation aligns well with the current designs for a centralised logger. I could not import the testcontainers.Logging interface due to circular import dependencies, so I had to define a local targeted interface.

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	fb9eeda
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/6753176427cd670008189887
😎 Deploy Preview	https://deploy-preview-2912--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle 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]

Hi @KenxinKun thanks for the fix. I'd go with the default testcontainers.Logger instead, so the eventual refactor of the logging system that @stevenh and I are going to tackle would be easier. It's true that enabling passing a custom logger to this wait strategy would make it, but I wonder if this would need to be applied to any other wait strategy. For that reason, I'd ask you to change the current impl, and simply delegate to the default logger. But I'd like to listen to your opinion fist.

[stevenh]

I'm wondering if this is another case for where having the default client and the ability to configure it with things like a logger could be way to go @mdelapenya as I suspect it's unlikely you want a separate logger for different strategies?

[mdelapenya]

it's unlikely you want a separate logger for different strategies

Indeed, that's what I think too. @KenxinKun if we change this PR to avoid a custom logger for the wait strategy, using the library's built-in (and to be changed soon) logging infra, then this would be ready to merge. Thanks!

[KenxinKun]

Sure no problem it makes sense, I'll try to change it tomorrow.
For my own use case through which I discovered the bug I'd have preferred the single logger indeed.

I'm not at my laptop to check now, but I recall having an import circular dependency importing from the parent testcontainers directory. I'll double check tomorrow if it's the same case here or not but if it is, should I try to untangle it?

[KenxinKun]

Yeah as I suspected the wait package is imported here (at least) https://github.com/testcontainers/testcontainers-go/blob/main/container.go
so unless we remove that import we won't be able to import testcontainers.Logger for the wait strategy...

[KenxinKun]

Morning! So I just tried briefly to untangle this but there's a significant number of uses of the wait package in the top testcontainers package. At first I thought it was just interfaces (primarily wait.Strategy and from the response of that one also wait.StrategyTarget), but there's also direct uses of wait.ForAll, wait.ForListeningPort and wait.ForExposedPort which complicates a possible refactoring 🤔

Thoughts:

• If it was just the interfaces I would have moved them to the top testcontainers package, and added an alias inside the wait package with a deprecation note (i.e. keep the "core" interfaces in the top package)
• The use of specific implementations of Strategy as listed above makes this difficult since it seems obvious we want to keep those implementations inside wait
• So without doing some likely problematic breaking changes it feels difficult to just import testcontainers.Logger into wait... The current implementation at least allows an injection but is it possible to move the logger to its own package? That would allow importing from elsewhere. For current definitions in the top package, perhaps type aliases would do the trick?

Let me know your thoughts @mdelapenya and @stevenh before I make further changes

[mdelapenya]

@KenxinKun sorry for making you going in circles with the wait and core packages 🤦 Indeed, the core uses wait so not possible.

What you suggested about having a logger makes sense, and we do have it in our backlog for an eventual v1 work, please see https://github.com/orgs/testcontainers/projects/13/views/1?filterQuery=log&pane=issue&itemId=87805987.

If you're interested in helping out with that, please let us know, as @stevenh would like to tackle the work on a more consistent logger, and you could collaborate in doing that.

[KenxinKun]

Sure happy to help, we use this library so much!