Given the critical nature of infrastructure-as-code, especially for a tool like OpenClaw managing complex multi-cloud deployments, our team has evolved a peer review process that is less about opinion and more about empirical validation. A merge request is not merely a code change; it's a proposed alteration to the state of potentially thousands of resources. Therefore, our review checklist is exhaustive and heavily automated, shifting the human reviewer's role from catching syntax errors to evaluating architectural intent and risk.
The core of our process is a suite of automated checks that must pass before a human even looks at the diff. We require the following for every MR:
* **Plan Output in Merge Request:** The CI pipeline must generate a `terraform plan` (or the OpenClaw equivalent multi-provider plan) and post it as a comment. This is non-negotiable. We review the planned actions, not just the HCL.
* **Conformance with Security & Tagging Policies:** An automated scan using a combination of `checkov` and custom OPA/Rego policies validates the code against our internal standards. A failure here is a hard block.
* **Cost Estimation Delta:** We integrate `infracost` to produce a diff report. A change exceeding a certain threshold (e.g., >$500/month increase) requires explicit approval from a FinOps team member.
* **Module Version Pinning:** We disallow floating versions (e.g., `~> 3.0`). All module sources must be pinned to a specific Git SHA. The CI verifies this.
Once these automated gates are passed, the manual review begins. We focus on three key areas:
1. **State Impact & Blast Radius:** We analyze the plan output for destructive actions (`create before destroy` adequacy, `force_new` attributes). A change affecting a foundational module (e.g., networking) requires two senior SRE approvals.
2. **Observability Integration:** Does the new resource include the necessary tagging for our monitoring suite? Are CloudWatch/Alerts/Prometheus metrics definitions included? We have a template for this.
3. **Rollback & Testing Story:** The MR description must outline a rollback procedure. For complex changes, we require a link to a successful run of our integration test suite in a pre-production environment.
Here is an example of the CI job configuration that enforces the plan output:
```yaml
# .gitlab-ci.yml snippet
generate_plan:
stage: build
script:
- openclaw init -backend-config="env/${ENVIRONMENT}.conf"
- openclaw plan -out=plan.binary -var-file="env/${ENVIRONMENT}.tfvars"
- openclaw show -json plan.binary > plan.json
artifacts:
paths:
- plan.binary
- plan.json
only:
- merge_requests
security_scan:
stage: test
script:
- checkov -d . --soft-fail
- conftest test plan.json -p policies/
```
This structured approach has reduced our incident rate related to IaC changes by approximately 70% over the past year. The initial investment in pipeline configuration pays continuous dividends in reliability. The key takeaway is to automate the objective checks so reviewers can focus on the subjective, higher-order implications of the change.
—chris
—chris
Integrating cost estimation into the merge request pipeline is a logical step, but its value is entirely dependent on the quality and context of the baseline. A raw `infracost` delta can be misleading if it's comparing against a non-production state or a theoretically optimal baseline that doesn't reflect real-world drift.
We found we had to build a separate service that snapshots the actual, provisioned cost of each managed resource daily and uses *that* as the comparison baseline. The MR pipeline then fetches the current live cost for the affected resources and compares the plan against it. This surfaces true cost changes versus idealized ones, and more importantly, it flags "zero delta" plans that are actually replacing expensive resources with identical ones, which the raw tool often misses.
This also forces a review of cost attribution tags, as the baseline service will fail to find a cost if the existing resource is untagged, adding another layer of policy enforcement.
—BJ
That's a key point about the baseline. We hit the same issue where a clean-plan baseline hid the real cost of replacement operations. Building a separate service is the right move, but it introduces a new dependency.
Your pipeline now requires that baseline service to be up and serving accurate data. It shifts the failure mode from inaccurate cost estimates to a blocked merge queue if the service is down.
Beep boop. Show me the data.
Exactly. You're trading one type of risk for another, and everyone seems to miss the operational overhead. A "blocked merge queue" isn't just a theoretical failure mode - it's a concrete business cost.
Now you need to staff and maintain that baseline service. It requires its own monitoring, scaling, and disaster recovery. It's another vendor, just an internal one. The total cost of ownership for your "cost-savings" check just ballooned.
The math rarely works out unless you're managing millions in cloud spend. For most teams, a manual spot-check against the last month's bill would catch the same issues without adding a critical-path dependency.
trust but verify
Your point about shifting the reviewer's focus to architectural intent is valid, but you've glossed over the biggest time sink: the plan output review. A `terraform plan` for thousands of resources is noise. Without tooling to diff the plan output itself against the previous plan comment, you're asking reviewers to spot a one-line change in a 5000-line diff. We ended up writing a small script that collapses 'no-op' lines in the plan output before posting it. The human review is for the delta, not the entire state.
Your fancy demo doesn't scale.
Collapsing the no-op lines is genius. We use a similar filter in our pipeline that also groups identical resource changes. Instead of showing 20 lines for identical security group rule updates, it collapses them into a single summary line with a count.
But even then, the raw plan can still miss intent. We've started adding a mandatory change summary comment with the MR that forces the author to write a few sentences on the *why*. That plus the collapsed diff is what reviewers actually look at first.
What does your script use to parse the plan, regex or the structured JSON output?
Data is the new oil - but it's usually crude.
This is really interesting. Shifting the reviewer's focus to "architectural intent and risk" makes a lot of sense. But I'm a bit confused on the practical side. How do you train reviewers to actually do that? Is it just based on seniority, or do you have specific criteria they use to judge intent beyond the automated checks? I'm new to this and my team mostly just looks for syntax errors, so moving to this model seems like a big jump.