fix: pin Terraform 1.15.5 for all Nix platforms - #25799
Conversation
e6ff6fe to
4734aa4
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.6.0 | Round 4 | Last posted: Round 4, 10 findings (2 P3, 7 Nit, 1 Note), APPROVE. Review Finding inventoryFindings
Cross-check notesCRF-1 convergence: Hisoka and Chopper independently flagged the same exit 0 issue from different angles. Hisoka focused on CRF-2 interaction with CRF-7: If the lookup table is adopted, the tautological naming ( CRF-3/4/5 pattern: Three instances of the same pattern (comment trailing clause restates code). Downgraded from Gon's P2 to Nit: these are convention/style issues. The comments are accurate and explain why; the extra words are marginally redundant. CRF-8 downgrade: Mafu-san rated P3 for commit atomicity. Kite explicitly said "Change 3 is a one-line mechanical fix that doesn't warrant its own PR." Downgraded to Nit: the fix is one line, documented in the commit body, and practically zero risk. CRF-9 folded into CRF-8: The shfmt version observation provides context for CRF-8 but is not actionable on its own. Pariston, Kite, Mafuuu, Bisky: No findings. Pariston performed adversarial analysis and could not construct a failure scenario. Kite traced both Makefile callers and confirmed correct behavior. Mafuuu performed contract tracing on all three changes. Contested and acknowledgedCRF-8 (Nit, check_commit_metadata.sh:90) - Unrelated shfmt fix bundled in commit
CRF-9 (Note, check_commit_metadata.sh:90) - shfmt version unverified
CRF-6 (Nit, commit) - Commit scope
Round logRound 1Panel. 2 P3, 6 Nit, 1 Note. 9 findings total. Reviewed against 12520ee..4734aa4. Round 2No panel (code unchanged). Author responded to all 9 findings: 6 claimed fixed (no push), 1 acknowledged/deferred, 1 contested (CRF-8, closed by orchestrator), 1 acknowledged with evidence (CRF-9, closed). Head SHA unchanged: 4734aa4. Panel deferred to round 3 after push. Round 3Churn guard: PROCEED. 6 findings addressed in d797d32, all verified by panel (6 of 6 reviewers confirmed). CRF-6/8/9 unchanged (closed/deferred in R2). 1 new Nit (CRF-10). Reviewed against 12520ee..d797d32. Round 4Churn guard: PROCEED. CRF-10 addressed in 426c178 (renamed About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Well-designed PR with defense in depth: the Nix pin fixes the root cause while the generate.sh guard prevents wrong testdata even if the pin is bypassed. The PR description is exemplary, tracing the causal chain from Nix fallback through version mismatch to testdata contamination. Multiple reviewers attempted to find a simpler alternative and concluded both layers are needed.
"I tried to build a case against this PR and couldn't." (Pariston)
2 P3, 6 Nit, 1 Note.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
1 similar comment
|
/coder-agents-review |
There was a problem hiding this comment.
Thanks for the thorough responses. All 9 findings addressed in comments.
CRF-8 (contested): accepted. The shfmt fix was a direct blocker to committing the primary change on macOS (pre-commit hook rejection). The causal link through the development workflow makes the bundling reasonable.
CRF-9 (acknowledged): closed. pinnedPkgs.shfmt resolving to 3.13.1 in the nix-shell verifies the trigger condition. The Note's concern was legitimate; the author provided the missing evidence.
CRF-6 (acknowledged): deferred to merge squash. No action needed before push.
CRF-1, 2, 3, 4, 5, 7: author claims fixed but head SHA is unchanged (4734aa4, same as R1). Verification deferred until the push lands. Panel will run on the updated code.
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
All R1 findings verified fixed in d797d32. Six reviewers independently confirmed: the lookup table is idiomatic Nix, the --upgrade guard correctly differentiates intent, and the comment edits are concise. One new Nit.
"Both sentences carry non-obvious information. First: what the committed data contains. Second: consequence. Minimum-draft is equivalent length. Kept." (Gon, verifying CRF-4 fix)
0 P3 remaining (both resolved), 1 new Nit, 6 verified fixed, 3 closed/deferred from R2.
CRF-6 (commit scope) remains deferred to merge squash.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 10 findings resolved across 4 rounds. CRF-10 verified: hashes renamed to releases in 426c178. Three reviewers confirmed no new issues.
Final tally: 2 P3 fixed, 7 Nit fixed (1 contested and accepted, 1 deferred to merge squash), 1 Note closed. Clean mechanical pass from Netero.
CRF-6 (commit scope) remains deferred to merge squash.
🤖 This review was automatically generated with Coder Agents.
426c178 to
5e3f892
Compare
The terraform_1_15_5 derivation previously only handled linux/amd64, falling through to unstablePkgs.terraform on all other platforms. On macOS this meant a different Terraform version was used, which caused the version check in make pre-commit to trigger generate.sh, regenerating all testdata with the host platform's OS/arch (darwin/arm64) instead of the committed linux/amd64 values. Three changes: 1. flake.nix: add explicit linux_arm64, darwin_arm64, and darwin_amd64 cases with SHA256 hashes from the official HashiCorp release. Unknown platforms still fall back to unstablePkgs.terraform. 2. provisioner/terraform/testdata/generate.sh: guard full regeneration behind a Linux-only check. The committed testdata encodes linux/amd64 values from the coder_provisioner data source, so regenerating on macOS would permanently bake in darwin/arm64. The --check path still runs on all platforms so the version target can detect provider mismatches. Regeneration via CI or an explicit Linux run is unchanged. 3. scripts/release/check_commit_metadata.sh: fix a shfmt (>=3.13) false positive. The [install.sh] key in an associative array literal was parsed as floating-point arithmetic (a zsh-only feature). Moving it to a post-declaration assignment satisfies the stricter parser without changing runtime behavior.
5e3f892 to
c5e395d
Compare
The terraform_1_15_5 derivation previously only handled linux/amd64, falling through to unstablePkgs.terraform on all other platforms. On macOS this meant a different Terraform version was used, which caused the version check in make pre-commit to trigger generate.sh, regenerating all testdata with the host platform's OS/arch (darwin/arm64) instead of the committed linux/amd64 values.
Three changes:
flake.nix: add explicit linux_arm64, darwin_arm64, and darwin_amd64 cases with SHA256 hashes from the official HashiCorp release. Unknown platforms still fall back to unstablePkgs.terraform.provisioner/terraform/testdata/generate.sh: guard full regeneration behind a Linux-only check. The committed testdata encodes linux/amd64 values from the coder_provisioner data source, so regenerating on macOS would permanently bake in darwin/arm64. The --check path still runs on all platforms so the version target can detect provider mismatches. Regeneration via CI or an explicit Linux run is unchanged.scripts/release/check_commit_metadata.sh: fix a shfmt (>=3.13) false positive. The [install.sh] key in an associative array literal was parsed as floating-point arithmetic (a zsh-only feature). Moving it to a post-declaration assignment satisfies the stricter parser without changing runtime behavior.Linear: DOCS-279