Creating container requests: separation of concerns

[mdelapenya]

Hi Testcontainers Gophers! We'd like to share in this discussion that, in the light of #590 (reply in thread), we are going to start changing how the ContainerRequest is created regarding basic and more advanced settings.

At this moment, the ContainerRequest struct establishes an almost 1-1 relationship with the Docker types, so that it's very straightforward to set up anything in the container directly from a field in the ContainerRequest. We think this is fine. At the same time, we see that exposing certain configurations in the ContainerRequest, such as advanced networking or advanced host-config settings, could lead to unexpected behaviors or potentials setup errors if those advanced fields are not properly used by the consumers of the library.

Another side effect of this 1-1 mapping with the Docker types is that, at some point, the public API of Testcontainers for Go could reach to a state that it's almost undistinguishable from the Docker client: all the Docker types could be eventually exposed in the ContainerRequest, creating a Frankenstein struct with multiple fields and types that would make its usage more difficult and less maintenable.

For those reasons, we are starting this discussion and offer a proposal that is working very well in other Testcontainers implementations, such as Java and .NET, which is creating a separation of concerns: basic fields and advanced fields:

• The basic fields would represent the vast majority of settings that users of the library are used to using: Image, mounts, env vars, wait strategies, etc., providing a nice and simple-to-use public API for Testcontainers. According to Docker docs, it could be very likely that the container.Config (portable information of the container) will live in this basic layer.
• The advanced fields, such as hostconfig settings (capAdd, capDrop, etc) and networking settings (IPAM, networks...) would be still available in the public API, but moved to a second layer of settings.

We think that this change will keep the public API simple and usable, but still powerful to provide advanced setups to experts users (see the advanced use case shared by @oktalz here: #590 (reply in thread)). Simple because the >80% of the regular use cases will be still and directly available in the ContainerRequest, and powerful because the ContainerRequest will expose a mechanism to setup those advance settings.

At the same time, the maintenance work on the library will be significatively reduced, as contributors won't need to send PRs to add/expose fields in the ContainerRequest, but just pre-configure the request with their own settings using a callback mechanism.

Something like this is what we have in mind:

type ContainerRequest struct {
	//...
	// PreCreationCallback allows advanced setting modifying the Docker types from its signature: container.Config and network.EndpointSettings
	PreCreationCallback func(*container.HostConfig, map[string]*network.EndpointSettings)
}

which will be internally used like this:

func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerRequest) (Container, error) {
	//...
	if req.PreCreationCallback != nil {
		req.PreCreationCallback(hostConfig, endpointConfigs)
	}

	resp, err := p.client.ContainerCreate(ctx, dockerInput, hostConfig, &networkingConfig, platform, req.Name)
	if err != nil {
		return nil, err
	}
	//...

At the moment the callback appears in the public API, we'd like to start deprecating "advanced" settings from the container request, as it'd be possible to set them up using the pre-creation callback.

As always, we'd love to hear your feedback.

Cheers!

[HofmeisterAn]

It looks like we are using a list of callbacks in Java and .NET. TBH, I do not see any advantages of a list, @eddumelendez @kiview do you know any? To keep the languages consistent, I suggest nonetheless a list of callbacks.

[kiview]

The API in Java works such, that withCreateContainerCmdModifier() adds a modifier to the (unordered) set of modifiers. The usage of a set can make sense in the case of modules already adding a modifier in their implementation (such as e.g., Consul) while still allowing users to add their own modifier without risk of conflict.

It is implemented like this at least since 2017 (see testcontainers/testcontainers-java@7d6d200) and I find it a reasonable API in the context of modules.

[HofmeisterAn]

Good point. I was not thinking about overriding a base configuration 👍 .

[mdelapenya]

Adding here a Slack thread:

@hhsnopek
1 day ago
Just saw this today - I'll expand on this in the discussion thread, but has functional options been considered for the design instead of methods?

[mdelapenya]

@mdelapenya
30 minutes ago
I like this approach, although I see the benefit of the functional opts

[mdelapenya]

@cyberbeast
30 minutes ago
If the ability to test the effects of each individual option (or even a combination of them) is important, then I’d go for functional options, But if the behavior can translate 1:1 with docker’s representation (and thus testing of these effects happening on the docker codebase) then the structs representation is sufficient. (edited)

[mdelapenya]

@mdelapenya
25 minutes ago
that's my feeling, that at the end we are mapping docker types in a 1:1 relationship

[mdelapenya]

@hhsnopek
5 minutes ago
I could see both as an option, 1:1 mapping with functional options. The primary reason is that the 1:1 mapping will end up being a lot of scaffolding/definition ahead of time that could heavily rely on defaults. For advanced use-cases users might opt for their own custom functional option or configuring the struct on their own. I often find packages which only give the ability to use one or the other painful to work with because they both require dig deep into the package for modifications.
Giving the option to have NewRequestContainer(ctx, opts ...RequestContainerOption) which applies defaults and the supplied options and public access to the RequestContainer struct itself will enable both. Tie that together with CreateContainer and only requiring the RequestContainer which could hold hooks or arguable be added after container is created. I would prefer a state where the tc-go api refined for better composure so that the developer can define them in any way they see fit.

[mdelapenya]

@hhsnopek @cyberbeast here you have a preliminary branch with functional opts that apply to GenericContainer, RunContainer, CreateContainer and ReuseOrRecreateContainer. Early feedback is more than welcome 🙏

[mdelapenya]

I think we can close this discussion, as it has already been implemented