Skip to content

Keep earlier values when withBindMounts, withResourcesQuota or withBuildArgs is called again - #1625

Draft
ankit1324 wants to merge 1 commit into
testcontainers:mainfrom
ankit1324:fix/with-calls-merge
Draft

ankit1324 wants to merge 1 commit into
testcontainers:mainfrom
ankit1324:fix/with-calls-merge

Conversation

@ankit1324

Copy link
Copy Markdown

Summary

Most with* methods that take a list or map add to what earlier calls set. Calling one of these three again threw away earlier configuration:

  • withBindMounts assigned hostConfig.Binds, so a second call dropped the first call's mounts. It now appends, like withExtraHosts, withUlimits and withSecurityOpt. This matches Java, where addFileSystemBind adds to the container's binds.
  • withResourcesQuota wrote both Memory and NanoCpus on every call, and wrote undefined for the field that was left out. So .withResourcesQuota({ memory: 0.5 }).withResourcesQuota({ cpu: 1 }) lost the memory limit. It now sets only the quotas passed in.
  • GenericContainerBuilder.withBuildArgs replaced the earlier args. It now merges them by key, where a later value for the same key wins, and it returns this like its siblings.

Modules pick this up through core, with no module changes. Bind mounts a user adds are no longer dropped when RedisContainer/ValkeyContainer (withPersistence) or QdrantContainer (withConfigFile) add their own bind in start(). A user's withBindMounts call also no longer removes the cgroup bind that K3s sets in its constructor.

The docs now say which methods add and which replace: one sentence on each of the three methods, and a short note under "Starting a container" that names the methods on each side. The withBindMounts section also says that mounting the same target twice fails when the container starts.

Verification

Each new test fails on main and passes with this change:

  • generic-container.test.ts, "should add bind mounts when called more than once". On main: expected 'cat: can't open '/tmp/test.txt'…' to contain 'hello world'.
  • generic-container-resources-quota.test.ts, "should only change the quotas given when called more than once". On main: expected +0 to deeply equal 536870912.
  • generic-container-dockerfile.test.ts, "should add build arguments when called more than once". On main: Failed to build image. The first call's VERSION was dropped, so the build tried FROM node:not-existent.

Results with the change:

  • generic-container.test.ts and generic-container-resources-quota.test.ts pass.
  • In generic-container-dockerfile.test.ts, all tests pass except the existing "should not pull existing image without pull policy". That test listens for any pull event on the Docker host, and "should use pull policy" in the same file pulls node:10-alpine concurrently. The new build adds one more concurrent build, and locally that made the overlap much more frequent (7 of 8 runs, against 1 of 5 on main). With CI=true, so vitest's retry: 3 applies, the file passed 11/11 on both runs, with one retry of that test each time. Making the two pull-event tests non-concurrent would fix it; I left that for a separate PR.
  • Module suites, all passing: redis 30/30, valkey 21/21, qdrant 5/5, chromadb 5/5, k3s 3/3. Qdrant, chromadb and k3s timed out on the first run while a large ollama/ollama pull was stalled on my Docker host. They passed on rerun, and none of their tests add binds that would collide.
  • npm run format, npm run lint, npm run knip and npm run check-compiles are clean.

Not breaking

A single call on a GenericContainer behaves as before, and the only type change is withBuildArgs returning this, which is narrower. What changes:

  • Calling one of these methods twice now keeps the earlier values, where before the later call replaced them: both sets of bind mounts, the earlier quota when the later call leaves it out, and both sets of build args.
  • K3sContainer binds /sys/fs/cgroup in its constructor. A user's withBindMounts call used to replace that bind and now keeps it. A user who binds /sys/fs/cgroup themselves now gets Docker's Duplicate mount point error.
  • RedisContainer/ValkeyContainer with withPersistence plus a user bind to /data, and QdrantContainer with withConfigFile plus a user bind on the config file path, now fail with Duplicate mount point. Before, the module's bind silently replaced the user's.
  • Calling start() a second time on those Redis, Valkey or Qdrant containers adds the module's bind again, and Docker rejects the duplicate. This is the start()-accumulation problem tracked in Make start() safe to call more than once on the same container #1520.

I've labelled this bug + patch like the issue, since the replace behaviour was undocumented and silently dropped configuration. The cases above can turn a silently ignored setting into an error, though, so whether this ships as patch or minor is your call.

Closes #1527

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make repeated with* calls merge consistently

1 participant