Skip to content

feat: upgrade dependencies and migrate deprecated APIs to new ones#139

Closed
kezhenxu94 wants to merge 8 commits intomainfrom
upgrade-deps-and-migrate-deprecated-apis
Closed

feat: upgrade dependencies and migrate deprecated APIs to new ones#139
kezhenxu94 wants to merge 8 commits intomainfrom
upgrade-deps-and-migrate-deprecated-apis

Conversation

@kezhenxu94
Copy link
Copy Markdown
Member

Summary

  • Upgrade testcontainers-go from v0.11.1 to v0.40.0
  • Upgrade docker/docker from v20.10.7 to v28.5.2
  • Upgrade k8s.io/* packages from v0.22.2 to v0.35.1
  • Upgrade sigs.k8s.io/kind from v0.27.0 to v0.31.0
  • Migrate deprecated LocalDockerCompose API to new ComposeStack API
  • Replace deprecated Docker types.* with new container.*, network.*, events.*, image.* packages
  • Update RESTMapper.NewShortcutExpander to use warning callback

Test plan

  • go build ./... passes
  • go test ./... passes

- Upgrade testcontainers-go from v0.11.1 to v0.40.0
- Upgrade docker/docker from v20.10.7 to v28.5.2
- Upgrade k8s.io/* packages from v0.22.2 to v0.35.1
- Upgrade sigs.k8s.io/kind from v0.27.0 to v0.31.0
- Migrate deprecated LocalDockerCompose API to new ComposeStack API
- Replace deprecated types.EventsOptions with events.ListOptions
- Replace deprecated types.Container* types with container.* types
- Replace deprecated types.Network* types with network.* types
- Update RESTMapper.NewShortcutExpander to use warning callback
- Add parseComposeFile() to parse compose files directly with yaml.v3
- Add loadEnvFromFile() to load environment variables from files
Copilot AI review requested due to automatic review settings February 14, 2026 05:05
@kezhenxu94 kezhenxu94 marked this pull request as draft February 14, 2026 05:06
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR upgrades several core dependencies (testcontainers-go, docker client, Kubernetes libs, kind) and migrates the codebase off deprecated APIs, especially around docker-compose orchestration and Docker SDK type packages.

Changes:

  • Migrates deprecated Testcontainers LocalDockerCompose usage to the modules/compose stack API.
  • Updates Docker SDK usages from deprecated types.* to newer container.*, network.*, events.*, and image.* packages.
  • Updates Kubernetes RESTMapper shortcut expander usage to the newer callback-enabled API.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
internal/util/k8s.go Updates RESTMapper shortcut expander to new signature with warning callback.
internal/components/setup/kind.go Migrates Docker image list/pull to types/image option types.
internal/components/setup/compose_provider.go Updates Docker container/network types and expands wait.StrategyTarget implementation surface.
internal/components/setup/compose_listener.go Updates Docker events options/types and adjusts event message ID access.
internal/components/setup/compose.go Reworks compose startup to use compose module stacks and adds compose/env parsing helpers.
internal/components/cleanup/compose.go Migrates compose cleanup to compose module stack teardown.
go.mod Upgrades module dependencies and adds replace directives for Docker archive compatibility.
Comments suppressed due to low confidence (1)

internal/components/setup/compose_provider.go:185

  • inspectContainer duplicates the same ContainerInspect call added in Inspect. To reduce duplication and keep behavior consistent, consider implementing inspectContainer by calling Inspect (or remove one of them if only one is needed).
func (c *DockerContainer) inspectContainer(ctx context.Context) (*container.InspectResponse, error) {
	inspect, err := c.provider.client.ContainerInspect(ctx, c.ID)
	if err != nil {
		return nil, err
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 54 to 58
func (c *ComposeContainerListener) Listen(consumer func(container *ComposeContainer)) error {
containerEvents, errors := c.client.Events(c.ctx, types.EventsOptions{
containerEvents, errors := c.client.Events(c.ctx, events.ListOptions{
Filters: filters.NewArgs(
filters.Arg("type", "container"),
filters.Arg("event", "start"),
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client.Events returns channels; downstream handling should not rely on len(errCh) to detect failures and should also handle channel closure (ok checks) to avoid spinning on closed channels. Consider restructuring Listen to propagate the first error deterministically (e.g., via select on errCh) and exit the goroutine when channels close.

Copilot uses AI. Check for mistakes.
@@ -68,27 +69,43 @@ func ComposeSetup(e2eConfig *config.E2EConfig) error {
return err
}
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Docker client created here is never closed. The Docker SDK client holds underlying HTTP connections; please defer cli.Close() after successful creation (similar to pullImages in kind.go) to avoid leaking resources in long-running processes/tests.

Suggested change
}
}
defer cli.Close()

Copilot uses AI. Check for mistakes.
Comment on lines +100 to +103
envVars, err := loadEnvFromFile(profilePath)
if err != nil {
return fmt.Errorf("load env file error: %v", err)
}
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: this returns a propagated error but uses %v instead of %w, which prevents callers from unwrapping the underlying error.

Copilot uses AI. Check for mistakes.
Comment thread go.mod
Comment on lines +236 to +240
// Compatibility fix for docker/cli which imports github.com/docker/docker/pkg/archive
// but the package has been moved to github.com/moby/go-archive
replace github.com/moby/go-archive => github.com/moby/go-archive v0.1.0

replace github.com/docker/docker/pkg/archive => github.com/moby/go-archive v0.1.0
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This self-replace (replace X => X v...) is effectively a version pin but is non-idiomatic and easy to misread as a no-op. Prefer adding an explicit require github.com/moby/go-archive v0.1.0 (or letting go mod tidy select) and keep replace only for the actual path substitution that’s needed.

Copilot uses AI. Check for mistakes.
Comment on lines 110 to 111
// Listen container create
listener := NewComposeContainerListener(context.Background(), cli, services)
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback passed to listener.Listen (below) mutates the outer err variable from another goroutine, which is a data race and can overwrite the function’s control-flow error value. Use a local variable inside the callback to handle/log errors without assigning to the outer err.

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +78
composeServices, err := parseComposeFile(composeConfigPath)
if err != nil {
return fmt.Errorf("parse compose file error: %v", err)
}
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These errors are formatted with %v, which loses wrapping semantics and makes errors.Is/As harder upstream. Prefer %w when returning an error you’re propagating (as done elsewhere in this file for other failures).

Copilot uses AI. Check for mistakes.
}

// No networks found
return "", nil
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When no networks are found, this returns an empty IP with a nil error. That can lead to confusing downstream failures (treating "" as a valid IP). Prefer returning a non-nil error when the container has no network/IP information available.

Suggested change
return "", nil
return "", fmt.Errorf("container has no network/IP information available")

Copilot uses AI. Check for mistakes.
Comment on lines +298 to +302
func (c *DockerContainer) Exec(ctx context.Context, cmd []string, options ...exec.ProcessOption) (int, io.Reader, error) {
cli := c.provider.client
response, err := cli.ContainerExecCreate(ctx, c.ID, types.ExecConfig{
Cmd: cmd,
Detach: false,
response, err := cli.ContainerExecCreate(ctx, c.ID, container.ExecOptions{
Cmd: cmd,
Detach: false,
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exec accepts options ...exec.ProcessOption and returns an io.Reader, but the implementation currently ignores all options and never attaches to capture stdout/stderr (it later returns a dummy reader). This can break callers/strategies that rely on ProcessOption behavior or output content. Consider applying options to the exec configuration and using ContainerExecAttach (or otherwise documenting/removing the output return).

Copilot uses AI. Check for mistakes.
wu-sheng added a commit that referenced this pull request Apr 14, 2026
…ndencies

- Delete compose_provider.go (601 lines) — custom DockerContainer/DockerProvider
  re-implementation replaced by native testcontainers-go APIs
- Delete compose_listener.go (85 lines) — Docker event listener no longer needed;
  containers are accessible after stack.Up() via stack.ServiceContainer()
- Rewrite compose.go to use compose.NewDockerComposeWith + stack.Up/ServiceContainer
- Rewrite cleanup/compose.go to use stack.Down
- Upgrade testcontainers-go v0.11.1 → v0.42.0
- Upgrade docker/docker v20.10.7 → v28.5.2
- Upgrade k8s.io/* v0.22.2 → v0.35.3
- Upgrade sigs.k8s.io/kind v0.27.0 → v0.31.0
- Upgrade spf13/cobra v1.8.0 → v1.10.2
- Remove docker-compose v1 binary install from CI (v0.42.0 uses Docker Compose v2 plugin)
- Update action.yaml Go version to 1.26

Closes #139

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wu-sheng added a commit that referenced this pull request Apr 14, 2026
Split Build workflow into lint-and-test + build-platform (ubuntu/windows/macos)
matrix jobs. Enable CGO for native builds. Update Dockerfile with build-essential
for CGO. Change `make build` to build for current OS only (CI matrix handles
cross-platform).

Based on #139

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@wu-sheng
Copy link
Copy Markdown
Member

Closed in favor of #146, which completed the full migration to testcontainers-go v0.42.0 with all dependency upgrades, multi-platform CI build matrix, and CGO support originally proposed here.

@wu-sheng wu-sheng closed this Apr 15, 2026
@wu-sheng wu-sheng deleted the upgrade-deps-and-migrate-deprecated-apis branch April 15, 2026 02:27
kezhenxu94 added a commit that referenced this pull request Apr 15, 2026
…146)

* feat: migrate to testcontainers-go v0.42.0 and upgrade all major dependencies

- Delete compose_provider.go (601 lines) — custom DockerContainer/DockerProvider
  re-implementation replaced by native testcontainers-go APIs
- Delete compose_listener.go (85 lines) — Docker event listener no longer needed;
  containers are accessible after stack.Up() via stack.ServiceContainer()
- Rewrite compose.go to use compose.NewDockerComposeWith + stack.Up/ServiceContainer
- Rewrite cleanup/compose.go to use stack.Down
- Upgrade testcontainers-go v0.11.1 → v0.42.0
- Upgrade docker/docker v20.10.7 → v28.5.2
- Upgrade k8s.io/* v0.22.2 → v0.35.3
- Upgrade sigs.k8s.io/kind v0.27.0 → v0.31.0
- Upgrade spf13/cobra v1.8.0 → v1.10.2
- Remove docker-compose v1 binary install from CI (v0.42.0 uses Docker Compose v2 plugin)
- Update action.yaml Go version to 1.26

Closes #139

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore: add multi-platform CI build matrix and CGO_ENABLED support

Split Build workflow into lint-and-test + build-platform (ubuntu/windows/macos)
matrix jobs. Enable CGO for native builds. Update Dockerfile with build-essential
for CGO. Change `make build` to build for current OS only (CI matrix handles
cross-platform).

Based on #139

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: remove compose.Wait(true) to avoid requiring all containers healthy

compose.Wait(true) maps to `docker compose up --wait` which requires ALL
containers to remain running and healthy. This is too strict for e2e tests
where some containers may not have healthchecks or may exit after init.
Port readiness is already handled by WaitForService() with per-port
wait strategies.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore: remove containerd image store disable workaround

No longer needed — testcontainers-go v0.42.0 uses Docker Compose v2
plugin which fully supports the containerd image store.

Reverts the workaround added in 8c21e43 (#140).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: add manual port waiting after compose up

WaitForService strategies only execute when compose.Wait(true) is passed,
but compose.Wait(true) is too strict (requires all containers healthy).
Instead, manually wait for each port via TCP dial after Up() returns,
matching the old code's behavior of waitPortUntilReady().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: restore DOCKER_API_VERSION negotiation in action.yaml

Docker SDK v28.5.2 defaults to API v1.51, but GitHub Actions runners
may only support up to v1.48. Set DOCKER_API_VERSION to the daemon's
supported version to ensure compatibility.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: add logging to port waiting and try immediately before polling

Add INFO-level logs for port wait start/completion and DEBUG-level for
retry attempts. Also try connecting immediately before the first ticker
interval to avoid unnecessary 500ms delay when ports are already ready.
Log when a service has no ports to wait for.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: wait for Docker healthcheck before port readiness check

After stack.Up(), wait for each container's Docker healthcheck to pass
(if defined) before doing TCP port checks. This matches the old behavior
where docker-compose up -d would respect depends_on: condition: service_healthy.

Also restored internal container port check (/proc/net/tcp + nc + /dev/tcp)
from the old WaitPort implementation to align with original behavior.

The wait sequence is now:
1. stack.Up(ctx) — start containers
2. waitForHealthy — poll Docker health status until healthy (if healthcheck defined)
3. waitForPort — external TCP dial + internal container check
4. Export env vars

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: only wait for healthcheck on services with ports

The old code only waited for services that had ports: in docker-compose.yml.
Services without ports were completely skipped. The healthcheck wait was
incorrectly running on ALL services, causing timeouts on services like
init containers or senders whose healthchecks depend on other services.

Now: services without ports get log streaming only. Services with ports
get healthcheck wait + port wait + env export (matching old behavior).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: remove healthcheck waiting, use only TCP+internal port check

The old code never checked Docker healthcheck status. It only did
TCP dial + internal /proc/net/tcp check. Many compose files use
healthchecks with tools (nc) not available in the container image,
causing Docker to mark containers as unhealthy even when the
application is running fine.

Remove waitForHealthy entirely to match old behavior exactly:
port-level waiting only, no Docker healthcheck dependency.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: restore build target to build all platforms for release

The build target must build all platforms (windows/linux/darwin) because
release-bin depends on it. Each platform build uses CGO_FLAG to enable
CGO only for native builds.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: build only current platform per CI matrix runner

Each matrix runner (ubuntu/windows/macos) now builds only its own
platform instead of all three redundantly. The `make build` target
is preserved for release which needs all platforms.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* ci: simplify build matrix by moving OS-to-target mapping into matrix include

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants