fix: address CI failures and FIXMEs in moby migration#2
Closed
aausch wants to merge 15 commits intothaJeztah:migrate_mobyfrom
Closed
fix: address CI failures and FIXMEs in moby migration#2aausch wants to merge 15 commits intothaJeztah:migrate_mobyfrom
aausch wants to merge 15 commits intothaJeztah:migrate_mobyfrom
Conversation
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>
The IsZero() branch was inverted during the moby migration, causing the HTTP wait strategy to inspect the container when a specific port was requested and vice versa. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The jsonmessage.DisplayJSONMessagesStream and term.GetFdInfo calls were commented out during the moby migration. The equivalent APIs exist at github.com/moby/moby/client/pkg/jsonmessage and github.com/moby/term. Without this, build errors are silently swallowed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stdcopy.NewStdWriter is no longer exported from moby. Add a local stdWriter that produces the same Docker-multiplexed frame format (8-byte header + payload), so that stdcopy.StdCopy can still demux stdout/stderr when reading logs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The old nat.Port type (a plain string) accepted empty strings:
SplitProtoPort("") returns ("", ""), so Port() == "" and no container
port matches, yielding "not found". The empty-string guard preserves
that behavior while avoiding a ParsePort error.
Add a test to verify MappedPort("") returns ErrNotFound.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Commit 05e8f91 added github.com/moby/term as a direct dependency in the root go.mod. All modules that depend on the core need their go.sum updated to include the transitive moby/term and Azure/go-ansiterm checksums, otherwise they fail at build time with: missing go.sum entry for module providing package github.com/moby/term Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
During the moby migration, two test mocks were incorrectly updated:
1. TestHttpStrategyFailsWhileGettingPortDueToUnexpectedContainerStatus:
Status was changed from "dead" to container.StateRunning. The test
intent is to simulate a dead container — fixed to container.StateDead.
2. TestHttpStrategyFailsWhileRequestSendingDueToUnexpectedContainerStatus:
Status was correctly set to container.StateDead, but the expected
error message was changed from "dead" to "running" — reverted.
Both tests originally used Status: "dead" (bare string). The old
docker/docker container.State.Status was a plain string field; the new
moby type uses the ContainerState enum. The migration should have used
container.StateDead ("dead"), not container.StateRunning ("running").
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The moby migration changed client.NetworkList() return type from []network.Inspect (a slice) to network.List (a struct). The test called require.Len() on the struct, which fails because len() can't be applied to a struct. The .Items field (already used on the next line) holds the actual slice. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MergeCustomLabels previously validated and merged in a single pass. If a reserved-prefix key was encountered partway through iteration, some keys would already be merged while others would not. Since Go map iteration order is non-deterministic, this produced flaky test results — the test assumed "B" was always merged before the invalid LabelLang key was encountered. Split into two passes: validate all keys first, then merge with maps.Copy. On error, dst is now guaranteed to be unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After container termination, Docker may not have fully disconnected endpoints from the network yet. The moby client surfaces this as "has active endpoints" on NetworkRemove. This race was latent with the old docker/docker client but is now consistently triggered. Retry up to 3 times with a 500ms delay when the error indicates active endpoints remain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three issues from the moby migration in the ollama local process: 1. Inspect() populated Networks with a "bridge" entry, but the old code used an empty map (bridge info was in the deprecated NetworkSettingsBase.Bridge field). Restored empty map. 2. Tests compared port.Proto() (network.IPProtocol) to "tcp" (string). The old nat.Port.Proto() returned string; the new network.Port.Proto() returns network.IPProtocol. Fixed to compare against network.TCP. 3. Tests compared port[0].HostIP (netip.Addr) to "127.0.0.1" (string). The old PortBinding.HostIP was string; the new one is netip.Addr. Fixed to compare against netip.MustParseAddr(testHost). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SkipIfDockerDesktop only checked for Docker Desktop by OS name, but colima also runs Docker inside a Linux VM where host networking doesn't work. Detect colima via DOCKER_HOST containing "colima" and skip early. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aausch
force-pushed
the
aausch/fix/moby-migration-fixes
branch
from
26376eb to
ab76469
Compare
Owner
|
Thanks! My branch needed a rebase, and I had some pending changes, so I rebased your branch and added it in my PR (probably the PR will need some squashing / cleaning up, but let's see what CI says now 😄) |
Author
🤦 I think my linters misfired |
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Addresses the CI failures and unresolved FIXMEs from the moby modules migration (testcontainers#3591). Each fix is an independent commit:
fix: correct inverted port condition in HTTP wait strategy—wait/http.go:194had!ws.Port.IsZero()instead ofws.Port.IsZero(), causing the HTTP wait strategy to take the wrong code path (inspect vs MappedPort)fix: re-enable build log streaming with moby jsonmessage API—jsonmessage.DisplayJSONMessagesStreamwas commented out during migration; without consuming the build response stream, images were never built, causing "No such image: UUID" errors across many testsfix(ollama): restore stdcopy multiplexing for local process—stdcopy.NewStdWriteris no longer exported from moby; added a localstdWriterthat produces the same Docker-multiplexed frame format sostdcopy.StdCopycan still demux stdout/stderrchore: resolve FIXME comment on MappedPort empty string handling— documented that the oldnat.Porttype accepted empty strings (verified via SplitProtoPort), addedTestMappedPortEmptyStringdeps: go mod tidy across modules for moby/term— propagatedmoby/termdependency to all modulesfix: restore correct container state in HTTP wait strategy tests— migration changedStatus: "dead"tocontainer.StateRunninginstead ofcontainer.StateDeadfix: use network.List.Items in network test assertion—NetworkListnow returnsnetwork.List(struct), not a slicefix: make MergeCustomLabels validate all keys before merging— Go map iteration order made the old single-pass validate+merge flaky; split into validate-then-mergefix(nebulagraph): retry network removal on active endpoints race— containers not fully disconnected before network removal; retry up to 3x with 500ms delayfix(ollama): correct local process Inspect and test type mismatches—Networksshould be empty map,Proto()returnsnetwork.IPProtocolnotstring,HostIPisnetip.Addrnotstringfix: skip host network tests on colima—SkipIfDockerDesktopdidn't account for colima (also a VM-based Docker environment where host networking doesn't work)Related issues
🤖 Generated with Claude Code