chore!: use testcontainers.Run function everywhere #3174
Conversation
mdelapenya
changed the title
* main: feat!: add options when creating RawCommand (testcontainers#3168) chore(deps)!: bump github.com/docker/docker from 28.1.1+incompatible to 28.2.2+incompatible (testcontainers#3194) feat(couchbase): adding auth to couchbase initCluster functions to support container reuse (testcontainers#3048) chore(deps): bump github.com/containerd/containerd/v2 (testcontainers#3167)
* main: chore: bump ryuk to 0.12.0 (testcontainers#3195)
* main: fix: workaround for moby/moby#50133 when reusing container in testcontainers.GenericContainer. (testcontainers#3197) feat(kafka,redpanda): support for waiting for mapped ports without external checks (testcontainers#3165)
|
@stevenh I think this is ready to review. I know it's a large one, but it applies consistently all the changes commit by commit. I decided to send it in one single PR instead of multiple ones because I'm deprecating the |
* main: fix: use PortEndpoint() in a few more modules (testcontainers#3203) fix: try to fix more IPv6 handling issues (testcontainers#3198) chore!: do not wait for all the exposed ports to be ready (testcontainers#3199)
| for k, v := range m.labels { | ||
| labels[k] = v | ||
| } |
|
|
||
| // labelMerger provides thread-safe operations for merging labels | ||
| type labelMerger struct { | ||
| mu sync.RWMutex |
There was a problem hiding this comment.
question: why does this need to be thread safe?
| env map[string]string | ||
| } | ||
|
|
||
| func defaultOptions() options { |
There was a problem hiding this comment.
question: could this just be WithEnv?
| var c *Container | ||
| if container != nil { | ||
| c = &Container{Container: container, password: req.Env["ARANGO_ROOT_PASSWORD"]} | ||
| c = &Container{Container: container, password: defaultOptions.env["ARANGO_ROOT_PASSWORD"]} |
There was a problem hiding this comment.
bug: this will always return the default value not the the overridden one, if the user customised it.
There was a problem hiding this comment.
I've debugged this and the opts.env receives the passed-in password from the option. All the modules that I'm adding an env field to the opts struct need access to one or more of those key/values to complete the state of the container (e.g. a password, a user), and with the WithEnv approach we simply has no access to the underlying map of env-vars. That's why I'm defining this intermediate map in the module opts struct, which is later contributed to the container opts using WithEnv. Does it make sense?
There was a problem hiding this comment.
Right now I get it.
The problem you have is you're changing the order of the options which can change the result, for example if you have a password option, followed by a WithEnv option which changes the value. With the current code the password option will get moved after the WithEnv and the resulting configuration would be incorrect. It's not going to be common case, but still breaking behaviour.
Instead we could make it easy to access the created container environment. It's currently possible via Inspect so a helper such as:
// Env gets the environment variables for the container.
func (c *DockerContainer) Env(ctx context.Context) (map[string]string, error) {
inspect, err := c.Inspect(ctx)
if err != nil {
return nil, fmt.Errorf("inspect: %w", err)
}
env := make(map[string]string, len(inspect.Config.Env))
for _, e := range inspect.Config.Env {
// Split the environment variable into key and value
parts := strings.SplitN(e, "=", 2)
if len(parts) == 2 {
// Add the key-value pair to the map
env[parts[0]] = parts[1]
} else {
// If no value is provided, set the value to an empty string
env[parts[0]] = ""
}
}
return env, nil
}Alternatively we could store the value from create request and expose by a simple method. The special case however would be a loaded container. We could still handle that.
// Env gets the environment variables for the container.
func (c *DockerContainer) Env() map[string]string {
return maps.Clone(c.env)
}Thoughts?
| func WithRootPassword(password string) testcontainers.CustomizeRequestOption { | ||
| return func(req *testcontainers.GenericContainerRequest) error { | ||
| req.Env["ARANGO_ROOT_PASSWORD"] = password | ||
| func WithRootPassword(password string) Option { |
There was a problem hiding this comment.
suggestion: delegate to WithEnv
| if container != nil { | ||
| c = &Container{Container: container} | ||
| if ctr != nil { | ||
| c = &Container{Container: ctr, user: defaultOptions.env["ARTEMIS_USER"], password: defaultOptions.env["ARTEMIS_PASSWORD"]} |
There was a problem hiding this comment.
bug: doesn't get the updated values.
|
|
||
| // WithCredentials sets the administrator credentials. The default is artemis:artemis. | ||
| func WithCredentials(user, password string) Option { | ||
| return func(o *options) error { |
There was a problem hiding this comment.
suggestion: delegate to WithEnv, more below
| return func(req *testcontainers.GenericContainerRequest) error { | ||
| req.Env["ACCEPT_EULA"] = "Y" | ||
| func WithAcceptEULA() Option { | ||
| return func(o *options) error { |
There was a problem hiding this comment.
suggestion: delegate to to WithEnv
modules/azure/eventhubs/options.go
Outdated
| func WithConfig(r io.Reader) testcontainers.CustomizeRequestOption { | ||
| return func(req *testcontainers.GenericContainerRequest) error { | ||
| req.Files = append(req.Files, testcontainers.ContainerFile{ | ||
| func WithConfig(r io.Reader) Option { |
There was a problem hiding this comment.
question: could you clarify why this change is needed?
There was a problem hiding this comment.
Mmm I think it was probably because of inertia... I double checked, and it can be kept as the core option.
Good catch
* main: chore: clarify image auth warning message for public images (testcontainers#3228) chore(deps): bump github.com/go-viper/mapstructure/v2 (testcontainers#3219) chore(deps): bump github/codeql-action from 3.28.16 to 3.29.2 (testcontainers#3222) chore(deps): bump mkdocs-include-markdown-plugin from 7.1.5 to 7.1.6 (testcontainers#3225) deps: update testcontainers/sshd image to 1.3.0 (testcontainers#3220) chore(deps): bump urllib3 from 2.2.2 to 2.5.0 (testcontainers#3214) deps: gopsutil and purego update (testcontainers#3217) docs: document copy from container (testcontainers#3215)
|
I'm converting this PR into draft as reference to sent smaller chunks and make progress with it. I'm listing all the smaller chunk PRs below: |
What does this PR do?
It applies the recently added
testcontainers.Runfunction in the core and in all the modules, deprecating the usage of theGenericContainerexposed function.For that, we needed to review all the modules, and:
moduleOptionsrepresenting the values in the original container requestmoduleOptssliceoptionsstruct, and a customOptiontype representing the module's own customisation types. If that struct and the Option type existed, we added a map[string]string for the env vars, changing all the existing options that could set env vars into the container request, into an Option type writing the passed-in values to the newenvfield in the options. Why? Because with this new approach, the module has no access to the container request within the scope of the Run method, so we need to change the functional opts and read those env vars from the options instead. The module's Run function now has to loop through the passed-in options and apply its own options.testcontainers.WithEnv), as they are processed internally in thetestcontainers.Runfunction.moduleOptions.The result is that the
testcontainers.Runfunction receives a slice of core options.This is considered a breaking change, as for those modules where we changed the existing options to the new custom type, the function signature changed. In practice, users that simply passed the options to the Run function has nothing to change, but we acknowledge that there could be users that are assigning these options to a variable, and here they will receive the breaking change. This could happen mostly in tests and test tables.
Apart from this, we noticed that several modules had their internal
Optiontype not returning errors, which was not consistent for our pattern to validate the options. Given this PR is already a BC for many modules, we took the chance to update those modules' options to return error, causing another breaking change.The first commit in this PR is deprecating the
GenericContainerfunction, so consumer would need to update to theRunfunction whenever possible. Their lint process should warn them at build/CI time.Modules that receive a Breaking Change
Why is it important?
testcontainers.GenericContainerhas been an important function in the library since the beginning, but we consider that havingtestcontainers.Runis much more idiomatic to the operations we want to do, which is no other than running a container. Also, it pairs with thedocker runcommand.With the addition of functional options for all the container request fields, we consider it's now possible to even deprecate the struct, move it to an internal package in the library, and always customise containers through the functional options APIs.