Repository navigation
test(integration): verify script ordering against compatible Coder versions - #552
Conversation
…rsions Add five cases that push templates using coder_script_order to a real Coder image. Four start a workspace agent inside the container and check, from a durable file and the build timings API, that startup scripts run in order, a dependent of a failed script is skipped, stop scripts run in order, and cancellation reports canceled outcomes. They also check that the stored dependencies match the template, so a renamed field on either side fails CI. The fifth proves an older Coder still builds the template. The agent cases are gated at v2.39.0 and skip on older images. The tests read the agent ID from the workspace because coderd generates its own on insert, and the compat template writes the agent ID to its output because Coder drops an agent that no resource references.
|
/coder-agents-review Please limit the review to P0, P1, and P2 issues only. |
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 2 | Last posted: Round 2, 30 findings (2 P2, 7 P3, 14 P4, 7 Nit), COMMENT. Review Finding inventoryFinding inventory, PR #552Findings
Contested and acknowledgedRound logRound 1Netero first pass: 1 P3, 1 P4, 1 Nit. No P0, panel proceeds. Round 2 updateBLOCKED. Churn guard: 3 addressed (CRF-4 in 6e63995, CRF-5 in e7fd688, CRF-16 by the 6e63995 rewrite), 27 silent. The requester posted "Please limit the review to P0, P1, and P2 issues only." a second time; churn guard classified it as a scope request, not a response to any finding. No reviewers, no Netero. Reviewed against 026b8a8..e7fd688. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds five integration cases that run coder_script_order templates against a real Coder image with an agent started in the container. 2 P2, 7 P3, 14 P4 and 7 Nit findings. The PR comment asked for P0 to P2 only: the two P2 findings, both intermittent failures in the new cases, are the ones inside that limit, and the rest are lower severity.
Out of scope (needs a ticket or explicit acceptance by a human):
.github/workflows/test.yml:5: the devel row, the only row that runs these cases today, has noscheduletrigger, so a Codermainrename of a dependency field is detected only on the next provider PR, whose author did not cause it.integration/integration_test.go:292: theminVersiongate treatsmain-...and prerelease tags as older than everyminVersion, so all gated cases skip on pinned preview images.integration/integration_test.go:302: the same tags select the deprecatedcoder templates create.integration/workspace-owner/main.tf: the session token is written to the output JSON, whichexecContainerlogs, so it is in CI logs.- coder/coder
agent/agent.go:2275: the agent reports its final lifecycle state before script timings and logs are flushed, so API clients cannot treatready,start_errororoffas "results stored". integration/integration_test.go:425:setup'srequire.Eventuallycondition callsrequirethroughexecContainer, so a Docker error waits the full 12 s.integration/integration_test.go:373: PID 1 in the Coder container iscoder server, which does not reap exited agents or script children; they stay zombies until the container is removed.
Notes:
coder-script-order-stopis the only case that exercisesphase = "stop".- Each
waitForAgentpoll logs the full agent JSON throughexecContainer, so a timeout produces about 120 copies before the failure line.
integration/integration_test.go:1
Nit [CRF-28] The commit subject is 75 characters, 82 after the squash suffix (#552). (Leorio)
Keep it under 72, for example test(integration): run coder_script_order cases against a real Coder.
🤖
🤖 This review was automatically generated with Coder Agents.
The agent reports ready or off before the goroutines that send script timings and logs have finished, so a single read right after the state change can come back empty.
code-server starts in parallel with clone_repo, so a signal sent as soon as clone_repo has started can also cancel code-server and fail the ok assertion on its timing.
|
/coder-agents-review Please limit the review to P0, P1, and P2 issues only. |
There was a problem hiding this comment.
The author reports that commits 6e63995 and e7fd688 fix the two P2 findings (timings and logs read before the agent stores them, and the cancel case signaling before code-server exits). Review panels will check those fixes in the next round.
This round has no review. 27 findings from the previous round have no reply on their threads and no change in the code. The request to limit the review to P0 to P2 does not say what happens to each of these findings. For each one, do one of these: fix it in this PR, link a GitHub issue, or reply on the thread with why it should stay as it is. A human can also reply that they accept every P3, P4 and Nit finding below as not fixed in this PR. The next round starts after one of these happens.
Findings that need a response:
- P3
README.md:88: README says the cases skip on old images; the gate compares theCODER_VERSIONstring - P3
integration/agent_test.go:251: case name repeated in the table,const name, and template/tmppaths - P3
integration/coder-script-order/main.tf:88:agent.idoutput is the Terraform ID the API does not know - P3
integration/integration_test.go:265: cancel case and log-text assertions tie provider CI to Coder agent wording - P3
integration/coder-script-order-stop/main.tf:25: comment sayswarmis needed to reachready - P3
integration/agent_test.go:255: wrong terminal lifecycle state fails without the agent log or timings - P3
integration/agent_test.go:259: a dependency-field rename fails the order check beforerequireDependenciesnames the cause - P4
integration/agent_test.go:345: cancel case never asserts the final lifecycle state - P4
integration/integration_test.go:233:v2.39.0repeated without a shared constant - P4
integration/agent_test.go:341:requireinside theEventuallycondition waits the full timeout on a Docker error - P4
integration/agent_test.go:74:stopAgentdoc states a purpose the cancel caller does not have - P4
integration/agent_test.go:236: "agent log" names two different things - P4
integration/agent_test.go:17: decode struct names drop parts of the codersdk names - P4
integration/agent_test.go:275:3*time.Secondduplicates the template'ssleep 3 - P4
integration/coder-script-order-skip/main.tf:30:timeout = 30on a script that exits at once - P4
integration/agent_test.go:298: status assertions carry no message naming the script - P4
integration/coder-script-order-compat/main.tf:44: comment contradicts thecoder_scriptreferences in the file - P4
README.md:88: skip sentence includes the compat case - P4
README.md:85: macOS build instruction comes after themakestep - P4
integration/agent_test.go:343: agent connect failure reads as a script failure with no log tail - P4
integration/coder-script-order/main.tf:33: code-server-before-clone-end assertion relies only onsleep 3 - Nit
integration/agent_test.go:7: stdlibslicesvsgolang.org/x/exp/slices - Nit
integration/integration_test.go:71:verifyfield comment carries usage advice - Nit
integration/integration_test.go:276: compat comment repeatsminVersion - Nit
integration/agent_test.go:214:scriptFordoc omits that it fails the test - Nit
integration/agent_test.go:155: "durable file" is a second name for the order log - Nit
integration/integration_test.go:1: commit subject is 75 characters
🤖 This review was automatically generated with Coder Agents.
Check dependencies before order, print the agent log tail on a wrong lifecycle state, and add the script name to status assertions. The checks on Coder's skip and cancel log wording are removed so a wording change on Coder main cannot fail provider CI.
… values Templates name their files after var.output_path so a copied template cannot share one. clone_repo waits for the unordered script instead of a sleep. Unused agent.id output, warm script, timeout and x/exp import removed.
There was a problem hiding this comment.
LGTM - great stuff 👍
I left just a nit and here's a couple of edge cases an agent found (up to you if you want to fix them or not).
-
Cancellation test can race with code-server finishing — agent_test.go:396. Seeing
start_code_server endproves the lastechoran, but the agent may not have collected the process’s exit yet.SIGTERMcan then produce a "canceled" timing, failing the later "ok" assertion. Wait for code-server’s successful timing before signaling. -
A polling callback still indirectly calls
require.NoError(t, ...)— agent_test.go:245.apiGetcallsexecContainer, which uses require on the originalt. A Docker error therefore terminates the callback goroutine. In this Testify version,EventuallyWithTcan interpret the untouched collector as success and return prematurely. The test remains failed, but polling stops and subsequent assertions see empty timings. HaveapiGetusetryExecContainerand return its error.
agent_test.go keeps the helpers any case with a live agent can reuse: starting and stopping the agent, reading the API, waiting for a lifecycle state and reading build timings. script_order_test.go holds the order log helpers, the dependency assertion and the four verify functions, which only make sense for the script ordering templates. No code changed, only its location.
apiGet now returns a Docker error instead of calling require. It is polled from testify's goroutine in waitForTimings, where a failed require ends the poll early and leaves the test to fail later on empty timings. The cancel case waits for the code-server start timing before sending the signal. The order log only shows the script's last line ran; the timing shows the agent saw the process exit and marked it ok, so the signal cannot cancel it as well.
|
Thanks for the two edge cases! Both fixed in 44a754a: |
2 participants
TL;DR
The provider and Coder each test their half of
coder_script_orderon their own, but nothing runs them together, so a renamed field on either side would pass every test and silently stop ordering scripts. This PR adds integration cases that push templates usingcoder_script_orderto a real Coder image, start a workspace agent inside the Coder container, and check that scripts ran in the declared order. On Coder images older than the feature the ordering cases skip with a message naming both versions, and one case proves such templates still build there.Refs https://linear.app/codercom/issue/PLAT-553/phase-4-verify-script-ordering-against-compatible-coder-versions
How to navigate this PR
provider/. About 360 lines of Go, five Terraform templates and four README sentences.integration/integration_test.go: one optionalverifyfield on the case table and one guarded call afterassertOutput. Then the five new table entries at the end of the table. Nothing else in the harness changes.integration/agent_test.go, the decode structs at the top: the few API fields the cases read. The package does not importcodersdk, for the same reason the harness never has.integration/agent_test.go, the helpers:startAgent,stopAgent,apiGet,agentIDFor,waitForAgent, then the small readers and matchers.agentIDForis the one to pause on, see Implementation.integration/coder-script-order/main.tf: the ordered template. The other four templates are variations of it.integration/agent_test.go, the fourverify*functions: one per agent case. Each starts the agent, waits, then reads the durable file, the build timings and the stored dependencies.README.md: how to run these locally.verifyScriptOrder, and inside it therequireDependenciescalls. That is the cross-repository contract check: if either repository renames a field in the plan JSON, those four calls fail with the script name and the missing dependency.Implementation
integration/integration_test.goverifyfunction to the case struct, called afterassertOutputwith the decoded output. Existing cases leave it nil.minVersion: "v2.39.0", the first release that enforces ordering. The compat case usesv0.0.0so it runs on every image.integration/agent_test.gostartAgentrunscoder agentin the background inside the Coder container. The agent reads its token throughCODER_AGENT_TOKEN_FILE, so the token never appears in the command the harness logs. The commandexecs the agent in a subshell so the PID file holds the agent's own PID. Each case gets its own log and data directories because the cases run in parallel in one container.stopAgentsendsSIGTERMto that PID. It never signals by process name, because the server in the same container is the same binary.apiGetcalls the API throughcurlin the container as the workspace owner and reports the HTTP status and body on failure. The session token is substituted by the container's shell at the moment of the call.agentIDForreads the agent ID from the workspace's latest build. Thecoder_agentresource'sidin Terraform is not the ID coderd stores: coderd generates its own on insert. The first run of these tests got a 404 on the Terraform ID, which is why this helper exists.waitForAgentpolls the agent's lifecycle state with a two-minute limit and prints the agent log tail on timeout, so a failure to connect reads differently from a failure to order.requireLineAfterchecks relative order only. Scripts that start together may write to the file in either order.coder-script-order: four startup scripts and two rules.clone_reposleeps three seconds so the unordered script has time to start before it ends. Every script appends a start line and an end line to one file per case.coder-script-order-skip: the same withclone_repoexiting 1.coder-script-order-stop: one startup script so the agent reachesready, two stop scripts and a stop rule.coder-script-order-cancel: the same as the ordered template withclone_reposleeping sixty seconds.coder-script-order-compat: the smallest template that uses the data source. It writes the agent ID into its output file on purpose. Coder drops an agent that no resource references, and the build then fails with "agent_id does not match any workspace agent or devcontainer subagent". The other templates reference the agent through their token files.local_sensitive_fileresources, not into the JSON output, so they stay out of the test log. The existingworkspace-ownercase puts its session token in the JSON; that case is not changed here.README.md: four sentences. On macOS the provider must be built for Linux first, because the binary runs inside the container. The other three say what the new cases do and when they skip.minVersionis what every existing case uses, andlatestbypasses it so the preview row fails, rather than skips, whencoder/codermainbreaks the contract.Expected Output
coder-preview:latest, with four startup scripts and rules that putinstall_toolsandpost_setupafterclone_repo, the durable file shows both dependents starting afterclone_repoends, andstart_code_server, which no rule names, starting before it ends. The build timings show the same order by timestamp, every statusok.install_toolsoncoder_script.clone_repowith requirementsuccess,post_setupon the same withcompletion, the other two none.clone_repoexits 1,install_toolsnever runs, its timing isskippedwith equal start and end, its log carriesSkipping script: dependency "Clone repo" failed.,post_setupstill runs because its rule asks forcompletion, and the agent ends instart_error.SIGTERMafter it is ready,stop_dbstarts only aftersave_stateends, and the build timings carry both stop-stage entries asok.SIGTERMwhileclone_repois still running,clone_repoiscanceledwith a positive interval,install_toolsandpost_setuparecanceledwith zero-length timings,start_code_serverisok, and the logs carryCanceled before starting.andCanceled while running.on the right scripts.skipping due to CODER_VERSION "v2.38.0" < minVersion "v2.39.0"and the step stays green.coder_script_orderstill imports and builds. On an older image the provisioner ignores the data source; on a newer one it stores the dependency. The compat case passes in both.Testing
integration/integration_test.go,TestIntegration: five new subtests.coder-script-orderchecks the first two bullets of Expected Output.coder-script-order-skipchecks the skip bullet.coder-script-order-stopchecks the stop bullet.coder-script-order-cancelchecks the cancel bullet.coder-script-order-compatchecks the last-but-one bullet and, through the harness's version gate, the skip-message bullet is what the other four produce on older images.GOOS=linux GOARCH=arm64 CGO_ENABLED=0 go build .:CODER_IMAGE=ghcr.io/coder/coder-preview CODER_VERSION=latest go test -count=1 -v ./integration/: all five new cases pass. Four pre-existing cases fail on this host only, because they compareprovisioner.osagainst the test host's OS (darwin) while the provisioner in the container reportslinux. They pass on the Linux CI runner and are not touched by this PR.CODER_IMAGE=ghcr.io/coder/coder CODER_VERSION=v2.38.0 go test -count=1 -v -run 'TestIntegration/coder-script-order' ./integration/: four skips, compat passes.gofmt,go vet ./integration,terraform fmt -check -recursive integration/all clean. Each new template passesterraform validateagainst the locally built provider through adev_overridesterraformrc.Manual Test
Docker Desktop 29.5.3 on an Apple silicon Mac. Preview image
v2.38.0-devel+200fa7dea7(built 2026-10-19 17:15 UTC). Release imageghcr.io/coder/coder:v2.38.0.coder-preview:latestcoder:v2.38.01. Whole package against coder-preview:latest
CODER_IMAGE=ghcr.io/coder/coder-preview CODER_VERSION=latest TIMEOUT_MINS=15 go test -count=1 -v ./integration/2. The five cases against coder:v2.38.0