chore!: migrate to moby modules#3591
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedToo many files! This PR contains 173 files, which is 23 over the limit of 150. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (66)
📒 Files selected for processing (173)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thaJeztah
force-pushed
the
migrate_moby
branch
9 times, most recently
from
0b5bb52 to
15c2930
Compare
|
@thaJeztah I had some extra cycles so went through as many of the outstanding issues as I could and tried to fix them. Hoping to help accelerate this migration. Each change should be in a self-contained commit just in case I got them wrong, to make it easy to review, cherry-pick, discard, etc. |
thaJeztah
force-pushed
the
migrate_moby
branch
2 times, most recently
from
ee10e27 to
10ff7ad
Compare
thaJeztah
force-pushed
the
migrate_moby
branch
4 times, most recently
from
e768eef to
35f9034
Compare
| portNum := mappedPort.Num() | ||
| assert.Positive(t, portNum, "Port number should be greater than 0") | ||
| assert.LessOrEqual(t, portNum, 65535, "Port number should be valid UDP port range") | ||
| assert.LessOrEqual(t, portNum, uint16(65535), "Port number should be valid UDP port range") |
There was a problem hiding this comment.
This assert can probably be dropped overall; it'd be extremely unlikely for a uint16 to be larger than 65535 😂
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Tests that create nginx containers exposing port 80 were incorrectly querying port 8080 via PortEndpoint, causing "port 8080/tcp not found". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The mock ContainerList returned an empty Items slice on success, causing findContainerByName to return nil and trigger unexpected retries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… bindings mergePortBindings now correctly preserves user-defined HostIP and HostPort from HostConfigModifier instead of discarding them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ContainerList now returns a struct, not a slice — use .Items for len(). Port.Num() returns uint16, not int — match type in assertion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The isRunning field was a plain bool read/written from multiple goroutines without synchronization. Use atomic.Bool to eliminate the data race detected by -race in TestContainerTerminationResetsState and Test_MultiContainerLogConsumer_CancelledContext. Note: a remaining race in TestContainerTerminationResetsState is upstream in moby/client cancelReadCloser, not in testcontainers-go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
preemtive 🥂 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb76dfedf8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if c.host == "localhost" { | ||
| cIP = netip.MustParseAddr("127.0.0.1").Unmap() | ||
| } | ||
| if c.host != "" { | ||
| cIP, err = netip.ParseAddr(c.host) |
There was a problem hiding this comment.
When c.host is "localhost", this code first maps it to 127.0.0.1 and then immediately reparses c.host with netip.ParseAddr, which fails for the hostname localhost. That makes Inspect() return an error for localhost-bound local Ollama runs, and any feature depending on Inspect (e.g., wait strategies or Ports) will fail even though the process is healthy.
Useful? React with 👍 / 👎.
| if p, err := network.ParsePort(port); err == nil { | ||
| ws.Port = p |
There was a problem hiding this comment.
This silently ignores parse failures and leaves ws.Port at zero, which WaitUntilReady treats as "use the lowest exposed port". A typo in WithPort(...) (for example an invalid protocol suffix) can therefore probe the wrong port and report readiness for an unintended endpoint instead of surfacing a configuration error.
Useful? React with 👍 / 👎.
* origin/main: chore(deps): bump github.com/moby/patternmatcher from 0.6.0 to 0.6.1 (#3628) chore(deps): bump github.com/shirou/gopsutil/v4 from 4.26.2 to 4.26.3 (#3627) chore: strip Gas Town overlay from CLAUDE.md (gt-p35) chore: add project instructions fix: auto-save uncommitted implementation work (gt-pvx safety net) fix(localstack): accept community-archive as a valid tag (#3601) chore!: migrate to moby modules (#3591) chore(deps): bump github.com/go-jose/go-jose/v4 in /modules/gcloud (#3632) chore(deps): bump actions/upload-artifact from 6.0.0 to 7.0.0 (#3625) fix(usage-metrics): include last release in the legend pop over (#3630) chore: update usage metrics (2026-04) (#3621)
|
@mdelapenya Waiting for next version. Thanks all. |
- testcontainers-go depends on docker/docker - eopa depends on docker/docker Both depend only in tests/examples on docker/docker , so CVE is not a vulnerability in skipper binary. testcontainers-go was fixed testcontainers/testcontainers-go#3591 eopa we are working on a fix open-policy-agent/eopa#370 --------- Signed-off-by: Sandor Szücs <sandor.szuecs@zalando.de>
| @@ -111,7 +114,7 @@ func (w *waitForSQL) WaitUntilReady(ctx context.Context, target StrategyTarget) | |||
| } | |||
| } | |||
|
|
|||
| db, err := sql.Open(w.Driver, w.URL(host, port)) | |||
| db, err := sql.Open(w.Driver, w.URL(host, port.String())) | |||
There was a problem hiding this comment.
Hi! I have a question about this change because my team is using the wait.ForSQL strategy.
Before, when port was nat.Port we could get the port number using port.Port() but now port.String() includes the protocol which means we have to parse the port ourselves. Does it make sense to either
- Change the port type in the callback to
network.Port
or - Instead use
port.Port()here to only pass the port number and exclude the protocol?
Thanks!
There was a problem hiding this comment.
Oh! Hm.. good question; I do recall that originally I changed some signatures to be more strongly typed (i.e., require network.Port as argument), but the number of places where a plain string was passed in, and now requiring to either use a network.MustParsePort or network.ParsePort was ... quite large, so I went back and made it loosely typed on the way in (and parse internally).
However this part is a callback, so the user isn't bothered by having to do the conversion, so perhaps we should change the signature to accept a network.Port as type here; while I expect most cases the port to be a TCP port, theoretically it could be a different protocol; I see there's at least a couple of tests passing in a port with a protocol; so passing ONLY the .Port() would mean we'd lose that information
testcontainers-go/wait/sql_test.go
Lines 15 to 19 in c251392
So perhaps changing it to a network.Port instead of a string would be best? That way the callback function wouldn't need to parse the port (again), and no information would be lost. Code that depended on nat.Port being a string (with extras) may need to be updated (either use port.String() or port.Port() depending on what's needed), but ... probably acceptable?
There was a problem hiding this comment.
- opened chore!: wait.ForSQL: change url callback to accept network.Network #3650 for consideration 😅
There was a problem hiding this comment.
Hey! Thanks for the quick response and PR!
Changing the type to network.Port makes sense to me!
5 participants
What does this PR do?
Migrate the Docker client dependency from the deprecated
github.com/docker/docker(v28, incompatible) to the newgithub.com/moby/mobymodular packages (github.com/moby/moby/api+github.com/moby/moby/client). This is a breaking change — several public API types and method signatures change.Breaking changes to the Testcontainers API
ContainerinterfacePortEndpointPortEndpoint(ctx, port nat.Port, proto string)PortEndpoint(ctx, port string, proto string)MappedPortMappedPort(ctx, nat.Port) (nat.Port, error)MappedPort(ctx, string) (network.Port, error)PortsPorts(ctx) (nat.PortMap, error)Ports(ctx) (network.PortMap, error)LivenessCheckPortsnat.PortSetnetwork.PortSetImageBuildInfointerface &FromDockerfilestructBuildOptions()build.ImageBuildOptionsclient.ImageBuildOptionsBuildOptionsModifierfunc(*build.ImageBuildOptions)func(*client.ImageBuildOptions)DockerClientmethodsInfoInfo(ctx) (system.Info, error)Info(ctx, client.InfoOptions) (client.SystemInfoResult, error)PingPing(ctx) (types.Ping, error)Ping(ctx, client.PingOptions) (client.PingResult, error)Events(<-chan Message, <-chan error)client.EventsResultDiskUsage(types.DiskUsageOptions) (types.DiskUsage, error)(client.DiskUsageOptions) (client.DiskUsageResult, error)RegistryLogin(auth registry.AuthConfig) (registry.AuthenticateOKBody, error)(client.RegistryLoginOptions) (client.RegistryLoginResult, error)Type replacements
docker/docker+docker/go-connections)moby/moby)nat.Port(string)network.Port(struct) or plainstringin method argsnat.PortMapnetwork.PortMapnat.PortSetnetwork.PortSetnat.PortBindingnetwork.PortBinding(.HostIPisnetip.Addr, notstring)build.ImageBuildOptionsclient.ImageBuildOptionsOther changes
MergeCustomLabelsto validate all keys before merging (eliminates flaky test)mergePortBindingsto preserve user-definedHostIP/HostPortfromHostConfigModifierDockerContainer.isRunningwithatomic.Boolto prevent data racesgo.mod/go.sumWhy is it important?
The
github.com/docker/dockermodule has been deprecated in favor of the properly-versionedgithub.com/moby/mobymodular packages. The new packages provide:+incompatible)network.Port) instead of string-basednat.Port, catching errors at compile timenetip.Addrfor host IPs instead of raw stringsStaying on the deprecated module blocks upgrading to newer Docker Engine APIs and accumulates tech debt as the ecosystem moves to the moby packages.
Related issues
How to test this PR
go test -race ./...make lint-allpasses across all 66 modulesTestTwoContainersExposingTheSamePort,TestContainerCreation,TestContainerCreationWithName— port handlingTestPreCreateModifierHook— port binding merge behaviorTestDockerProvider_waitContainerCreation_retries— container list result typeTestGenericReusableContainerInSubprocess— container list.ItemsusageTestUDPPortBinding— port type changesFollow-ups
TestContainerTerminationResetsStateandTest_MultiContainerLogConsumer_CancelledContextfail consistently with-racedue to a data race upstream inmoby/moby/client(cancelReadCloserinutils.go:139,152). ThecancelReadCloserstruct has an unsynchronized field written during construction and read during the cancel callback'sClose(), triggered viaContainerLogs(). This needs a fix in themoby/moby/clientpackage before this PR can merge with-raceenabled.