chore: support disabling reaper at the configuration level

[mdelapenya]

What is this PR doing?

This PR comes with a breaking change, regarding how the Docker client is obtained: the method to obtain an instance of the Docker client was public API, and now it's not.

The changes that drives that breaking change are the following:

• the NewDockerClient has been converted into private, as we consider exposing the Docker client is not a responsibility of this library.
• the newDockerClient function is not returning an instance of the testcontainers properties anymore, as the properties are now part of a global singleton, accessible from any part of the code with the functions explained below.

Besides that, the main intention of this PR was/is to initialise Ryuk/the reaper just once, not allowing to do it every time that a container is requested. In the past, before these changes, it was possible to create N containers and each of them would be able to define a strategy whether to skip the reaper or not. With this changes, we define the reaper disabled once. More specifically:

• reading the properties (and its tests) has been moved to an internal package. It's an implementation detail of the library how to read and set up the properties. A collateral benefit is being able to run the tests for this package, only.
• the testcontainers properties is initialised once as a global singleton, with a method to retrieve it, the public .Get() function. That's how any other code in the library is able to get the properties.
• the properties are loaded in a lazy manner: the first caller of the .Get() method will read them.
• the load of the properties is protected by sync.Once, therefore it guarantees it's executed just once :smile:
• we have defined the following property to control if the reaper/Ryuk is enabled/disabled: ryuk.disabled, which accepts boolean values.
• it's also possible to define the same behavior using an environment variable: TESTCONTAINERS_RYUK_DISABLED, which accepts boolean values.

And the reaper struct? It is represented by a singleton, but we have optimised it a little bit:

• the mutex.Lock/Unlock has been moved inside the first nil check, followed by another nil check
• meanwhile working in this refactor, we detected a blocking cycle when calling the reaper: when creating a network, the reaper is called to clean up resources. The implementation of the reaper is a Ryuk container, which will check for a reaper before it's run. As the check for isReaperNeeded is now part of the properties, at the moment that value is set to ryuk.disabled=false (default), then the mutext would lock infinitely: the network will wait for its reaper, which cannot continue creating its underlying container because of the lock has not been released. For that reason we had to add a check for the container request image: if the request is for the reaper, then skip.

In terms of the CI, we have created a separate pipeline that runs the same tests that the main one, but with reaper disabled. For that, we are setting up the library with a .testcontainers.properties file including ryuk.disabled=true. This step, but setting the value to false, has been added to the main pipeline, where Ryuk is enabled.

A side effect of this PR is that we are adding consistency to the existing tests:

• using the Nginx Docker image constant, always.
• identify which tests were leaking a container without removal, and terminate it using the terminateContainerOnEnd testing helper func.

Why is it important?

There is a few of them:

• cleaner APIs: exposing the properties and how the Docker client is generated is not the purpose of the library, therefore moving them to internals, or making functions private makes more sense.
• alignment with the testcontainers-java project as, in that project, it's not possible to define a reaper per container.
• leverage more and more the .testcontainers.properties file, which will allow adding optimizations to container execution.
• consistency in tests

Related issues

• Relates #549

How to test this

I used a Go project using testcontainers-go, and added a replace entry on its go.mod file to use the current workspace, so the local version is used.

When running the tests on that project, I verified that:

• if the properties defines Ryuk as disabled, the containers should live after the tests run (unless a defer clause destroys them)
• if the properties defines Ryuk as enabled, the containers should be reaped after the tests run.

[codecov[bot]]

Codecov Report

Merging #561 (5f655d4) into main (9860647) will decrease coverage by 36.57%. The diff coverage is 74.60%.

@@             Coverage Diff             @@
##             main     #561       +/-   ##
===========================================
- Coverage   68.99%   32.42%   -36.58%     
===========================================
  Files          22       12       -10     
  Lines        2174     1687      -487     
===========================================
- Hits         1500      547      -953     
- Misses        535     1057      +522     
+ Partials      139       83       -56     

Impacted Files	Coverage Δ
container.go	46.98% <ø> (-33.74%)	:arrow_down:
network.go	0.00% <ø> (ø)
testing.go	0.00% <0.00%> (ø)
docker.go	38.85% <66.66%> (-32.79%)	:arrow_down:
reaper.go	75.22% <80.43%> (-8.57%)	:arrow_down:
parallel.go	0.00% <0.00%> (-97.11%)	:arrow_down:
compose.go	0.00% <0.00%> (-74.05%)	:arrow_down:
file.go	0.00% <0.00%> (-52.95%)	:arrow_down:
mounts.go	50.00% <0.00%> (-50.00%)	:arrow_down:
docker_mounts.go	46.34% <0.00%> (-48.79%)	:arrow_down:
... and 15 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mdelapenya]

Just checked that the java library supports disabling the reaper at the envVars level, only. Therefore, I'm going to update this PR to remove the support at the properties level.

[HofmeisterAn]

Just checked that the java library supports disabling the reaper at the envVars level, only. Therefore, I'm going to update this PR to remove the support at the properties level.

TBH I do not like it. I think the configuration should be the same for both. It just makes it unnecessary difficult for developers if we add exceptions for settings. If the Resource Reaper should not be disabled, it should be mentioned explicit in the docs.

[mdelapenya]

Just checked that the java library supports disabling the reaper at the envVars level, only. Therefore, I'm going to update this PR to remove the support at the properties level.

TBH I do not like it. I think the configuration should be the same for both. It just makes it unnecessary difficult for developers if we add exceptions for settings. If the Resource Reaper should not be disabled, it should be mentioned explicit in the docs.

@HofmeisterAn is the ability to set the reaper at the envVar level only the thing that you don't like? I do prefer having both styles to configure the library: the properties and the env var.

[HofmeisterAn]

is the ability to set the reaper at the envVar level only the thing that you don't like?

I cannot say much about the Go implementation. My Go skills aren't that good.

I do prefer having both styles to configure the library: the properties and the env var.

I agree.

[mdelapenya]

Superseded by #941