Skip to content

test: migrate Docker orchestration to testcontainers-go#374

Open
ethanndickson wants to merge 1 commit into
ethan/bedrock-role-arnfrom
ethan/integration-testcontainers
Open

test: migrate Docker orchestration to testcontainers-go#374
ethanndickson wants to merge 1 commit into
ethan/bedrock-role-arnfrom
ethan/integration-testcontainers

Conversation

@ethanndickson

@ethanndickson ethanndickson commented Jun 26, 2026

Copy link
Copy Markdown
Member

Replaces the hand-rolled Docker SDK container lifecycle in integration/integration.go with testcontainers-go. StartCoder's signature and all its options are unchanged, so the acceptance and integration test callsites are untouched.

testcontainers handles image pulls, the mapped host port, readiness waiting, and cleanup, which lets us delete the pullImage and randomPort helpers and drop docker/docker as a direct dependency.

Closes PLAT-367.

Copy link
Copy Markdown
Member Author

@linear-code

linear-code Bot commented Jun 26, 2026

Copy link
Copy Markdown

PLAT-367

@ethanndickson

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a754e2e19

ℹ️ 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".

Comment thread integration/integration.go Outdated
Comment thread integration/integration.go Outdated
@ethanndickson ethanndickson force-pushed the ethan/integration-testcontainers branch from 0a754e2 to 1503379 Compare June 26, 2026 06:23
@ethanndickson

Copy link
Copy Markdown
Member Author

@codex review

re: Bind the Coder test port to loopback

Thanks — I dug into this. You're right that the old hand-rolled Docker setup explicitly bound the published Coder port to 127.0.0.1, while the testcontainers ExposedPorts path leaves the host bind address to Docker/testcontainers defaults.

I don't think we should take the loopback binding change as part of this migration, though. The idiomatic testcontainers API here is ExposedPorts plus wait.ForHTTP, and forcing a loopback bind requires dropping down into HostConfigModifier and importing the Moby API types directly. It also makes readiness less straightforward in some Docker environments because testcontainers may resolve the Docker host to a bridge/gateway address, while a loopback-bound published port is only reachable via 127.0.0.1.

The practical risk seems acceptable for this test harness: the instance is ephemeral, uses a random host port, is short-lived, and runs inside the test runner environment. If we want localhost-only published ports as a hard security requirement, I think that should be a separate hardening change with explicit handling for testcontainers host resolution across Docker environments.

I did keep the other fix from this review: cleanup and failure log collection are now registered immediately after GenericContainer returns and before asserting startup success, so a start/wait timeout won't leak the container or lose the logs.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1503379828

ℹ️ 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".

Comment thread integration/integration.go Outdated
@ethanndickson ethanndickson force-pushed the ethan/integration-testcontainers branch from 1503379 to 734112b Compare June 26, 2026 07:19
@ethanndickson

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 734112b862

ℹ️ 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".

@ethanndickson ethanndickson requested a review from johnstcn June 26, 2026 07:28
@ethanndickson ethanndickson self-assigned this Jun 26, 2026
@ethanndickson ethanndickson changed the title test(integration): migrate Docker orchestration to testcontainers-go test: migrate Docker orchestration to testcontainers-go Jun 26, 2026
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.

2 participants