Repository navigation
Conversation
[ENG-8760](https://stacklet.atlassian.net/browse/ENG-8760) What ---- - Adds `validate-min-terraform`, a composite action that reads a module's Terraform floor out of `required_version` and runs `terraform validate` at that release. - Sets the repository up for the tools that follow: one directory per tool, each with an `action.yml` and a `README.md`, plus a lint workflow, a prek config, a justfile and a README that records the layout. Why ---- Five of the AWS and Azure onboarding module repositories need this job, and the version of it that ENG-8760 wrote carries enough shell to be worth maintaining in one place. Each repository validates at the release pinned in `.tool-versions`, which sits near the top of the supported range, so nothing covered the bottom of it. That is how `required_version = ">= 1"` survived in a module that no release below 1.8 can even parse. The action derives the floor rather than taking it as an input, so editing the constraint moves what CI tests in the same commit. It runs terraform-docs to parse the configuration instead of matching a regex against one file, which is what the published actions do and what makes them wrong on a second `.tf`, a `.tf.json`, or a constraint split across lines. The terraform-docs image is pinned by digest because the project publishes no immutable releases, so both a tag and a release checksum can change underneath us. Callers pin by commit SHA. This repository publishes no release tags, and a floating ref would put the module repositories' CI under whatever lands here next. Testing ---- - [x] Ran the floor derivation against a module carrying three constraints at once, one of them in a `.tf.json` and one split across lines: it reports the highest lower bound, where the regex-based action reports an empty string that `semver` then reads as "any version". - [x] Ran it against all five onboarding modules, each of which resolves to the floor its `required_version` states. - [x] Checked the operator handling across a matrix: `>= 1.14.0`, `>= 1.14.0, < 2.0.0` and `~> 1.14` resolve; `!=`, a bare version and `>` are rejected; an upper bound on its own reports that the constraint states no lower bound. - [x] Checked that a container that fails to run prints docker's own error, rather than surfacing as an empty parse further down. - [x] `just lint` clean, with zizmor re-run under a token to confirm the five online audits schedule rather than skip. zizmor audits `action.yml` as well as the workflow. - [ ] Confirm the action runs end to end once this merges and the module repositories point at it. Docs ---- `README.md` lists the tools and says how to add one. The action carries its own `README.md` with usage, inputs, outputs and a note on the one case it cannot catch: `terraform validate` never evaluates `validation` blocks, so a floor that is only wrong inside one still passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@cubic-dev-ai please review |
`>= 1` is the constraint this ticket started from, and the parser rejected it outright. Terraform reads a short bound as the lowest release matching it, so it now pads to three components rather than failing. `--show requirements` overrides a `.terraform-docs.yml` that hides the requirements section, which would otherwise report a module with a constraint as declaring none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review raised three things the inline shell could not answer. The parse now lives in `read-floor.sh`, so shellcheck reads it and a test can run it without a runner. `test-read-floor.sh` covers every constraint form the parser accepts or rejects, and CI runs it. The floor now has a floor. The release this installs comes out of the configuration under test, so without a clamp a branch picks which Terraform runs in CI, and Terraform below 1.2.2 bundles a go-getter affected by HCSEC-2022-13. `minimum-floor` defaults there and callers can raise it. The parse container also drops to a read-only mount with no network, neither of which terraform-docs needs to read a configuration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parse was three mechanisms doing one job: a case statement for the operator, tr to strip whitespace, and two greps to validate what was left. Extending it meant touching all three. It is now one regex per term, and the version comparison is a tuple rather than a sort -V idiom. Splitting the derivation from the terraform-docs call makes the constraint cases unit tests. Fourteen of the sixteen cases no longer start a container, so the suite runs in a second and a new case costs nothing. It also retires a trap. The old loop read from a heredoc so that it ran in the current shell and its accumulator survived; rewriting it as a pipe would have silently emptied the result and failed every valid module. ruff replaces shellcheck, since no shell is left to check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Architecture diagram
sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Action as validate-min-terraform Action
participant TD as terraform-docs Container
participant Script as read_floor.py
participant TF as Terraform CLI
participant Runner as CI Runner
Note over Dev,Runner: NEW: validate-min-terraform composite action flow
Dev->>GH: Push/PR to main
GH->>Runner: Trigger CI workflow
Note over Runner: CI workflow runs lint + test jobs
Runner->>Runner: lint: prek (actionlint, zizmor, ruff)
Runner->>Runner: test: unittest discover
alt Test job
Runner->>Script: Run read_floor.py tests
Script->>Script: Parse constraint cases (no container)
Script->>TD: Docker run for fixture tests
TD-->>Script: Parse requirements
Script-->>Runner: Test results
end
Note over GH,Runner: Real module validation flow
Runner->>Action: Composite action starts
Action->>Action: Set working-directory
Note over Action: Step 1: Read floor
Action->>Script: Execute read_floor.py
Script->>Script: Read required_version from module
Script->>TD: Docker: terraform-docs json --show requirements
TD->>TD: Parse all .tf/.tf.json files
TD-->>Script: requirements JSON
Script->>Script: Derive highest lower bound
alt Valid floor found
Script->>Script: Check minimum-floor clamp
alt Floor >= minimum (1.2.2)
Script-->>Action: version output
else Floor < minimum
Script-->>Action: Error + exit 1
end
else No constraint or unparseable
Script-->>Action: Error + exit 1
end
Action->>Action: Write version to GITHUB_OUTPUT
Note over Action: Step 2: Setup Terraform
Action->>TF: Install terraform (version from floor)
TF-->>Action: terraform ready
Note over Action: Step 3: Validate
Action->>TF: terraform init -backend=false
TF->>TF: Install providers, fetch modules
Action->>TF: terraform validate
TF-->>Action: Validation results
Action-->>Runner: Step completed
Note over Dev,Runner: SECURITY: no secrets, no write permissions
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
145094a to
34049ab
Compare
| # including one in a .tf.json or split across lines. | ||
| - name: Read the declared Terraform floor | ||
| id: floor | ||
| shell: bash |
There was a problem hiding this comment.
A composite step has to declare a shell, so shell: bash itself cannot go. The hold-over was the logic on that line.
read_floor.py now writes version= to $GITHUB_OUTPUT itself, so the step is a bare invocation of the script. A test covers the file contents.
Filed by Claude on Cory's behalf.
|
|
||
| Runs `terraform validate` against a module at the oldest Terraform release the module says it supports. | ||
|
|
||
| A repository's lint job validates at the release pinned in `.tool-versions`, which is near the top of the supported range. Nothing covers the bottom of the range, so a module can declare `required_version = ">= 1.0"` and use syntax that no release below 1.8 parses. This action closes that gap. |
There was a problem hiding this comment.
@johnsca to be honest, given https://endoflife.date/terraform and terraform's relatively short support lifecycle, I'm left wondering whether we might not be better off simply enforcing a modern terraform minimum floor?
There was a problem hiding this comment.
The official support info from HashiCorp has 1.14.x getting fixes and security patches though August of next year, per the Support Lifecycle Addendum. Which is a bit complicated, so we could instead just follow the 2 year primary LTS window.
Should I change the action to figure out the minimum version based on the 2-year LTS window and validate with that and that the constraint falls within that LTS window instead? That would force us to keep it up-to-date instead of letting it drift like we had.
There was a problem hiding this comment.
Keeping it a literal rather than deriving the window at run time.
Deriving it means a network call to HashiCorp on every run, a new failure mode when that endpoint moves or rate-limits, and a check whose result changes with no commit to point at. A module that passes on Monday fails on Tuesday because a window rolled.
The literal is 1.14.0, with the reasoning and its ambiguity in the README beside it. If drift is the worry, a scheduled job that opens a pull request raising the default keeps the change reviewable.
Filed by Claude on Cory's behalf.
| | Name | Default | Description | | ||
| | --- | --- | --- | | ||
| | `working-directory` | `.` | Directory holding the module to validate. | | ||
| | `minimum-floor` | `1.2.2` | Refuse to run below this release. | |
There was a problem hiding this comment.
@johnsca Shouldn't the minimum floor be something supported and modern? The version you mention has been end of life for over 3 years and has a ton of security problems.
There was a problem hiding this comment.
Agreed, and changed. The default is now 1.14.0 rather than 1.2.2.
1.2.2 cleared one advisory, the go-getter issues reachable while init fetches modules. Nothing else recommended it.
1.14 rests on a judgement call. HashiCorp publishes no support window for the community CLI. Its support policy routes a version to a regime by release date, and 1.14 lands in the band still receiving fixes and security patches. The Support Lifecycle Addendum that band points at lists only Self-Managed products, so no row names a CLI line. We read the routing table as the applicable statement, which puts 1.14 at the oldest release with a published claim to fixes. If you read it differently, the input exists and the number is one line.
The README records that reasoning, and says the default wants revisiting whenever a new minor lands.
Filed by Claude on Cory's behalf.
| @@ -0,0 +1,3 @@ | |||
| just 1.40.0 | |||
There was a problem hiding this comment.
@johnsca this is very old - you may as well update it before shipping this? same with uv.
There was a problem hiding this comment.
Updated here and in the five module repositories: just 1.58.0, uv 0.12.23, and in the modules terraform 1.16.5 and tflint 0.64.0. just lint passes at all of them.
terraform-docs stays at 0.20.0. The docs job runs terraform-docs/gh-actions v1.4.1, which builds FROM quay.io/terraform-docs/terraform-docs:0.20.0. Moving it would have CI check the README against a different renderer than just docs uses.
The image this action parses with stays there too. 0.20.0 is the last release published as a multi-arch index, and later tags are per-architecture, so a digest pin would have to pick one.
Filed by Claude on Cory's behalf.
The three-constraint fixture could not fail on the constraint split across lines: the one in the .tf.json is higher, so dropping the multiline file leaves the expected floor unchanged. A second fixture makes the multiline form the deciding bound, where ignoring it yields 1.3.0 rather than 1.14.0. The tool table said the action validates at the declared floor without saying it refuses one below the configured minimum, and the note on `>` did not say why an exclusive bound names nothing to install. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clamp defaulted to 1.2.2, which closed one advisory and otherwise had nothing to recommend it: that line stopped receiving fixes years ago. It now defaults to 1.14.0, the oldest release still inside HashiCorp's support window, and the README records the rule and the review it needs when a new minor lands. The docker invocation is one shlex.split of an interpolated string rather than twenty lines of one argument each. read_floor.py writes the step output itself, so the action step is a bare invocation with no shell logic. just and uv in .tool-versions were both months behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`= 1.14.0` and a bare `1.14.0` both name a definite minimum, and Terraform reads them as the same constraint, so rejecting either was a gap rather than a guard. `!=` stays rejected, since it can rule out the bound another term states. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`>= 1.14.0, != 1.14.2` resolves to 1.14.0, since the exclusion does not touch the bound. `>= 1.14.0, != 1.14.0` still fails, because finding the next release above an excluded floor needs the release list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a52f36f to
e47cbf9
Compare
|
@cubic-dev-ai please re-review |
@johnsca I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 18 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant U as User Module Repo
participant W as GitHub Workflow
participant A as validate-min-terraform Action
participant P as read_floor.py
participant D as terraform-docs Container
participant T as Terraform CLI
participant CI as CI Pipeline
Note over U,CI: Composite Action: Validate Terraform Floor
U->>W: Trigger CI (push/PR)
W->>A: Invoke composite action
A->>P: Execute read_floor.py
P->>D: docker run (parse .tf files)
D->>D: Extract required_version constraints
D-->>P: JSON with requirements
P->>P: Parse constraints, derive floor
alt Floor below minimum
P-->>A: Error: below minimum floor
A-->>W: Fail action
else Valid floor
P->>P: Write version to GITHUB_OUTPUT
P-->>A: Return version
A->>T: Setup Terraform at floor version
A->>T: Run terraform init -backend=false
T-->>A: Init complete
A->>T: Run terraform validate
T-->>A: Validation result
A-->>W: Complete action
end
W-->>U: CI result (pass/fail)
Note over U,CI: Tests run locally via unittest
P->>P: Execute unit tests (no container)
P->>D: End-to-end tests with Docker
Branch protection sets require_code_owner_reviews on a public repo, and with no CODEOWNERS file that setting matches nobody. The other five repos in this change carry the same one-line file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ENG-8760
what
Adds
validate-min-terraform, a composite action that reads a module's Terraform floor out ofrequired_versionand runsterraform validateat that release, with the parse in its own script and a test suite over it. Lays out the repository for the tools that follow it: one directory per tool, each carrying anaction.ymland aREADME.md, alongside a lint workflow, a prek config, a justfile and a README recording the layout.why
Five of the AWS and Azure onboarding module repositories need this check, and the version of it that ENG-8760 wrote carries enough shell to be worth one copy rather than five. Those repositories validate at the release pinned in
.tool-versions, which sits near the top of the supported range, so nothing covered the bottom of it. That is howrequired_version = ">= 1"survived in a module that no release below 1.8 can parse.The action derives the floor instead of taking it as an input, so editing the constraint moves what CI tests in the same commit. It runs terraform-docs to parse the configuration rather than matching a regex against one file. The regex approach is what the published actions do, and it is why they misread a second
.tf, a.tf.json, or a constraint split across lines, the last of which yields an empty string that semver then reads as "any version".The terraform-docs image is pinned by digest, not by tag. The project publishes no immutable releases, so a tag and the checksum published beside a release can both change underneath us, while a digest is content addressed and cannot.
The release the action installs comes out of the configuration under test, so on a pull request the branch decides which Terraform runs in CI.
minimum-floorputs a bound under that, defaulting to 1.14.0. HashiCorp publishes no support window for the community CLI, and its support policy routes a version to a regime by release date, which puts 1.14 in the band still receiving fixes and security patches. That reading is a judgement call, recorded in the README alongside the number, and the default wants revisiting whenever a new minor lands. Callers can raise it to their own support floor.Two things the clamp does not solve, and the action cannot. A pull request still chooses which providers
initinstalls, and a provider plugin is a binary thatvalidateruns to read its schema. So the job must hold no secrets and no write permission, which is the shape the usage example and all five callers have. The same is already true of theterraform_validatepre-commit hook these repos run today, so this is not new exposure, but it is worth stating where someone will read it.The parse lives in
read_floor.pyrather than inline in the action. Inline, nothing linted it: the pre-commit actionlint hook only reads.github/workflows/, and a compositeaction.ymlis not a workflow. In a script a linter reads it and a test can run it without a runner.It is Python rather than shell because the work is parsing and comparing. One regex per term replaces a case statement, a
tr, and two greps that all had to agree, and the version comparison is a tuple rather than asort -Vidiom. Splitting the derivation from the terraform-docs call is what makes the constraint cases unit tests: fourteen of the sixteen no longer start a container.Callers pin by commit SHA, because this repository publishes no release tags and a floating ref would put five repositories' CI under whatever lands here next. If the repository grows enough tools to want versioned releases, that is the point to add tags and switch the callers over.
testing
.tf.jsonand one split across lines. It reports the highest lower bound, which is the one Terraform's owninitenforces.required_versionstates.>= 1.14.0,>= 1.14.0, < 2.0.0and~> 1.14resolve, while!=, a bare version and>are rejected with the term that could not be read, and an upper bound on its own reports that the constraint states no lower bound. Review on the module PRs raised the first of those.just testcovers every constraint form the parser accepts or rejects, a constraint split across lines where that is the deciding bound, the three-constraint fixture, a.terraform-docs.ymlthat hides the requirements section, a module with no constraint at all, a floor under the clamp, the step output written toGITHUB_OUTPUT, and a container that cannot run. CI runs the same command. The suite takes under two seconds, since most cases call the parser directly and start no container.>= 1was rejected outright, which is the constraint this ticket started from. A config hiding the requirements section made a module that declares a constraint look like one that declares none. The multiline fixture could not fail, because a higher bound in the sibling.tf.jsondecided the result either way. The clamp defaulted to a release nobody has patched in years.just lintclean, and zizmor re-run under a token to confirm its five online audits schedule rather than skip. The prek release that job installs now comes from.tool-versionsrather than being resolved at run time, so CI and a local run agree on it. zizmor auditsaction.ymlas well as the workflow.uses:pin names the commit on this branch, so it resolves as soon as the branch exists on GitHub, but it wants repointing at the commit onmainafter this merges.docs
README.mdlists the tools and says how to add one. The action carries its ownREADME.mdwith usage, inputs, outputs and the one case it cannot catch:terraform validateagainst a module directory treats every root variable as unknown and so never evaluatesvalidationblocks, which leaves a floor that is only wrong inside one still passing.This repository is not described in
cloud-infra, so its settings and required checks are unmanaged. Worth adding there separately.🤖 Generated with Claude Code