Closed 0xced closed 3 months ago
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.
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.
I haven't had the time yet, but I will review it in the next few days.
Great, I'll refrain from submitting new pull requests in the meantime. π
The PR is ready for a final review. @0xced your feedback is much appreciated (no hurry). I am happy to merge the PR afterward.
Awesome, I'll try to have a look this week.
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β¦
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.
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?
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.
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. π
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.
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. π
What does this PR do?
This pull request moves the logger configuration from the static
TestcontainersSettings
class to the AbstractBuilder class.Before:
After:
Why is it important?
This will enable a seamless integration with xUnit.net (both with
ITestOutputHelper
and withIMessageSink
).Related issues
Fixes #996