Conversation
|
There was a problem hiding this comment.
Pull request overview
This pull request successfully refactors the codebase to use the new moby/moby/client and moby/moby/api modules instead of the legacy docker/docker module. The goal is to reduce binary size by using the modularized Moby packages.
Key Changes
- Client initialization: Updated from
client.NewClientWithOpts()toclient.New()across Docker and Docker Swarm providers - API method signatures: Adapted to new API patterns including return value changes (e.g.,
ContainerStop,ContainerStartnow return results) and options structures (e.g.,client.ServiceUpdateOptionsnow wrapsVersionandSpec) - Data structure access: Updated to use new collection wrappers (e.g.,
services.Itemsinstead of direct slice access,spec.Container.Stateinstead ofspec.State) - Filters API: Migrated from
filters.NewArgs()toclient.Filters{}with the sameAdd()method pattern - Events API: Transitioned from dual-channel pattern to result object pattern (
result.Messagesandresult.Errinstead of separate channels)
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/testcontainers/pind/pind.go | Removed Docker-specific imports; critical container configuration commented out |
| pkg/sabliercmd/provider.go | Updated client initialization to use client.New() instead of client.NewClientWithOpts() |
| pkg/provider/dockerswarm/*.go | Updated imports, filters API, service list/inspect methods to use new collection wrappers, ServiceUpdate options structure |
| pkg/provider/docker/*.go | Updated imports, container methods to handle new return values, events pattern, inspect response structure |
| go.mod | Added moby/moby/api and moby/moby/client dependencies; moved docker/docker to indirect dependency |
| go.sum | Updated dependency checksums for new modules |
| Makefile | Added .PHONY: build directive for make target |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Comment on lines
+27
to
+35
| /*ConfigModifier: func(config *container.Config) { | ||
| // config.User = "podman" | ||
| }, | ||
| HostConfigModifier: func(hc *container.HostConfig) { | ||
| hc.Privileged = true | ||
| // Disable cgroup v2 for the container | ||
| hc.CgroupnsMode = "host" | ||
| hc.SecurityOpt = []string{"label=disable", "seccomp=unconfined", "apparmor=unconfined"} | ||
| }, | ||
| },*/ |
There was a problem hiding this comment.
The ConfigModifier and HostConfigModifier functions have been commented out, which removes important container configuration. These modifiers were setting:
Privileged = true- Required for running Podman inside DockerCgroupnsMode = "host"- Disabling cgroup v2 for the containerSecurityOpt- Security options for label, seccomp, and apparmor
Commenting out this configuration will likely cause the Podman in Docker container to fail at runtime. These settings should be preserved unless there's a specific reason to remove them (e.g., if the new moby/moby/client API provides an alternative way to set these).
Comment on lines
+10
to
12
| "github.com/testcontainers/testcontainers-go/wait" | ||
|
|
||
| "github.com/testcontainers/testcontainers-go" |
There was a problem hiding this comment.
[nitpick] The import statement for github.com/testcontainers/testcontainers-go/wait should be placed with other third-party imports after standard library imports, maintaining alphabetical order. Currently it's placed separately after the blank line, which breaks the import grouping convention.
Suggested change
| "github.com/testcontainers/testcontainers-go/wait" | |
| "github.com/testcontainers/testcontainers-go" | |
| "github.com/testcontainers/testcontainers-go" | |
| "github.com/testcontainers/testcontainers-go/wait" |
❌ 8 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
❌ 8 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
|
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
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.
This should reduce the size of the binary by using the new
moby/moby/clientandmoby/moby/apimodules.testcontainers-go is still using the
docker/dockermodule, waiting for: testcontainers/testcontainers-go#3496