Make the logger configurable with a new WithLogger method on the builders

[0xced]

What does this PR do?

This pull request moves the logger configuration from the static TestcontainersSettings class to the AbstractBuilder class.

Before:

TestcontainersSettings.Logger = logger;

After:

new TBuilderEntity().WithLogger(logger).Build()

Why is it important?

This will enable a seamless integration with xUnit.net (both with ITestOutputHelper and with IMessageSink).

Related issues

Fixes #996

[netlify[bot]]

✅ Deploy Preview for testcontainers-dotnet ready!

Name	Link
🔨 Latest commit	66ec29fa7963d0eaadb13fb2e0ef7afd7eca1d81
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-dotnet/deploys/65ef31dc4fd9ec0008539e1f
😎 Deploy Preview	https://deploy-preview-1100--testcontainers-dotnet.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.

[0xced]

Continuing discussion from https://github.com/testcontainers/testcontainers-dotnet/pull/1093#discussion_r1485208704 to here (where it belongs).

The PR does not contain the Xunit project, right? I think it is a good idea!

Indeed this pull request does include the new Xunit project. I will submit another pull request dedicated to the new Xunit integration.

A lot of developers will benefit from it. I have not taken a close look into the PR that involves the changes to the logging implementation yet. I took a quick look, and it is quite big.

It's big because it removes the ILogger parameter from all containers in all modules but the actual change in AbstractBuilder`4.cs is suprisingly small.

IIRC, I proposed to split it into two steps: one that provides an internal WithLogger(ILogger) builder method, and another that makes the necessary changes regarding logging the container runtime information and making the method public. After that, we can add the dedicated Xunit, package. WDYT? Splitting it makes reviewing much easier.

I tried but I don't see how it's possible to split in two steps. Again, if you ignore the required changes in the modules the core feature diff is not that big.

[HofmeisterAn]

I haven't had the time yet, but I will review it in the next few days.

[0xced]

Great, I'll refrain from submitting new pull requests in the meantime. 😉

[HofmeisterAn]

The PR is ready for a final review. @0xced your feedback is much appreciated (no hurry). I am happy to merge the PR afterward.

[0xced]

Awesome, I'll try to have a look this week.

[0xced]

I just pushed 1ee0e3e56358fef9e2e501741107cd86901989f1 which uses the configured ILogger for the resource reaper too.

I think the configured logger should also be passed to the PortForwardingContainer, I'm looking into it…

[HofmeisterAn]

I think the configured logger should also be passed to the PortForwardingContainer, I'm looking into it…

We can also change this with a follow-up PR. Probably, we simply need to replace the singleton property with a method that provides an ILogger overload.

[0xced]

OK, let's take care of PortForwardingContainer in a follow up PR.

I see that you removed the structured logging of runtime information that I had introduced. I re-introduced it in a slightly improved way in fef7faaf93c8b9e7ff199071011a22d2556f44ff. I think it makes the LogContainerRuntimeInfoAsync implementation much easier to read and also it's the right way to do structured logging. But if you don't like it, just drop the commit.

One last question: why did you introduce an empty TestcontainersSettings static constructor in e535cbec9dacc98a499efdcaaa6883f9db83f8cf?

[HofmeisterAn]

I think it makes the LogContainerRuntimeInfoAsync implementation much easier to read and also it's the right way to do structured logging. But if you don't like it, just drop the commit.

Can we please revert it? I really do not like it, and structured logging does not have a lot of benefits here. I would like to remove Logging anyway in the future. Logging the message immediately is much easier. Happy to merge the pull request afterward and publish a beta version.

[0xced]

OK fine, I’ll revert.

What about the empty TestcontainersSettings static constructor?

Happy to merge the pull request afterward and publish a beta version.

Could you please have a look at #1139 before publishing? I promise it’s much simpler than this one. 😉

[HofmeisterAn]

What about the empty TestcontainersSettings static constructor?

This guarantees lazy initialization (probably not really important here), see beforefieldinit.

Could you please have a look at #1139 before publishing? I promise it’s much simpler than this one. 😉

Yes, we can merge this before publishing a beta version too.

[0xced]

I just pushed 1bf2f9689197779419974c91fcf3d52bb9772a3c which removes the structure logging and I extracted the logging code into a new method for improved readability, and also catching only the exceptions when running GetSystemInfoAsync and GetVersionAsync.

This guarantees lazy initialization (probably not really important here), see beforefieldinit.

Wow, I had no idea, gotta agree with Mike Marynowski!

[...] controlling the timing of when you want a class to run its static initialization shouldn't be controlled entirely by the presence or absence of a static constructor, that's just so odd...

I think it's now ready to be merged. 😀