Infrastructure as Code and GitOps Questions

Direct answer

An automated PR-commenting system for Terraform plans needs three integration points working together: a CI webhook (or native CI-to-PR integration) that runs terraform plan and posts its output back to the specific pull request, a static-analysis step (tfsec or an equivalent) whose findings get merged into the SAME comment rather than a separate, easy-to-miss check, and a merge-blocking gate keyed off both the plan's destructive-change classification and the static-analysis severity. The single hardest design requirement, easy to get wrong, is presenting a genuinely large plan diff ACTIONABLY (a reviewer should see destructive changes FIRST, not buried in 40 unchanged resources) while never leaking a sensitive computed value (a generated password, a private key) into the PR comment where anyone with read access to the repo, not just the deploy pipeline, can see it.

Structured elaboration

Integration points. CI (GitHub Actions, GitLab CI, or equivalent) triggers on every push to an open PR, runs terraform plan -out=tfplan, and posts results via the platform's native PR-comment API (a GitHub App or bot account with SCOPED, comment-only permissions, not broad write access to the repo) rather than a generic webhook receiver that would need its own separate authentication and posting logic. tfsec (or Checkov, or an equivalent policy/security scanner) runs against the SAME plan or the raw configuration, and its findings are posted to the SAME PR comment thread, updated in place on each new push rather than appending a fresh comment every time, which would quickly bury the actual current state under a growing scroll of stale comments.

Presenting diffs actionably. A single comment, UPDATED (not re-posted) on every push, structured with the highest-risk information FIRST: a summary line up top ("3 to add, 2 to change, 1 to DESTROY"), an explicit, visually distinct callout for any destroy or destroy-and-recreate action (these are what a reviewer most needs to not miss), then the full plan detail collapsed behind a <details> disclosure so it is available without dominating the comment by default, then the tfsec/security findings, similarly severity-sorted with critical/high findings surfaced above informational ones.

Blocking merge on high-risk changes. A required status check (separate from the comment itself, since a comment alone cannot BLOCK anything) evaluates a policy: any DESTROY action on a resource tagged as production-critical, or any tfsec finding at CRITICAL severity, fails the check and blocks merge until either the plan changes or an authorized reviewer explicitly overrides with a recorded justification (never a silent bypass).

Avoiding leaking sensitive outputs in PR comments. Terraform's own sensitive attribute marking suppresses a value from PLAN CLI output, and the automation should PRESERVE that suppression when re-rendering the plan into a PR comment, never re-serializing the plan JSON in a way that bypasses the sensitive-value redaction Terraform itself already applies; additionally, run a SECOND, independent redaction pass (a regex/pattern scan for common secret shapes: long base64-looking strings, anything matching a known credential format) over the comment body before posting, as a defense-in-depth backstop for the case where a value is genuinely sensitive but was NOT marked sensitive in the provider schema (a real, recurring gap, not a hypothetical one).

Worked example

A concrete PR comment structure the automation posts and updates in place:

## Terraform Plan Summary
:warning: 1 resource will be DESTROYED (review carefully)
+3 to add, ~2 to change, -1 to destroy

### Destructive changes (review first)
- `aws_db_instance.legacy`: DESTROY (ForceNew: engine_version change)

### tfsec findings
- CRITICAL: aws_s3_bucket.uploads has no encryption configured (AWS018)
- LOW: aws_security_group.app allows egress to 0.0.0.0/0 (AWS009)

<details><summary>Full plan output</summary>

... (complete plan, sensitive values shown as (sensitive value) per Terraform's own redaction) ...

</details>

Merge is BLOCKED: 1 destroy on a production-tagged resource, 1 CRITICAL tfsec finding.

Trade-offs and pitfalls

• Common mistake: posting a NEW comment on every CI run instead of updating the SAME comment in place. On an actively-iterated PR (several pushes while addressing review feedback), this buries the CURRENT plan state under a scroll of stale ones, and a reviewer glancing at the PR can easily read an OUTDATED plan comment by mistake; updating in place is a small implementation detail with an outsized effect on whether the automation is actually trustworthy to read.
• Relying SOLELY on Terraform's own sensitive schema marking for redaction is a real, common gap, a provider attribute the schema author simply forgot to mark sensitive (or a genuinely NEW attribute added in a provider update before anyone reviewed its sensitivity) will render in plain text; the independent pattern-based redaction pass exists specifically to catch what schema-level marking misses, not as redundant caution.
• A merge-blocking gate keyed only on tfsec severity, without also weighing WHICH resource a destroy targets, both over- and under-blocks. A destroy on a genuinely disposable dev-environment resource does not need the same blocking rigor as a destroy on a tagged production-critical one; tagging resources explicitly for this purpose (rather than inferring criticality from naming conventions) keeps the gate meaningful rather than either noisy or too permissive.
• A silent override path (someone with sufficient repo permissions simply force-merging past a failing required check) defeats the entire control if it exists without an explicit, audited justification requirement; the override mechanism itself needs to be a deliberate, logged action, not merely "anyone with admin rights can route around this."

Direct answer

The workflow itself (branch, edit, commit, push, open a pull request, wait for CI, merge) is IDENTICAL in mechanics to an application-code change; what differs for infrastructure-as-code (IaC) is what the required CI validation actually checks (a terraform plan showing the real-world effect of the change, not just a test suite) and the weight given to review, since a merged infra change can directly reconfigure live, shared resources the moment it applies, with no separate build-and-deploy step standing between merge and real-world effect the way an application change usually has.

Structured elaboration

Step-by-step, using the question's own example commands as the backbone.

git checkout -b feat/update-vpc

Start from an up-to-date base branch and create a descriptively named feature branch; naming it after the CHANGE ("update-vpc") rather than a ticket number alone helps a reviewer understand intent from the branch list without opening the PR.

Edit the Terraform configuration (for example, widening a VPC's CIDR range in the relevant .tf file).

git add .
git commit -m "feat(vpc): increase cidr range"

A commit message following this project's convention: a concise subject describing WHAT and, ideally, enough context to infer why, referencing a ticket if one exists.

git push origin feat/update-vpc

Push the branch and open a pull request describing the intended change and its expected blast radius.

Running CI validations, the infra-specific step. The pipeline runs terraform fmt -check and terraform validate (syntax and internal consistency), then terraform plan, and posts the PLAN OUTPUT directly on the PR as a comment or check annotation, so the reviewer sees exactly what will change in the real infrastructure (which resources get created, modified in place, or destroyed and recreated) alongside the code diff, not just the code diff alone.

Review. The reviewer reads both the CODE diff and the PLAN output together; particular attention goes to any resource marked for DESTRUCTION or REPLACEMENT in the plan, since those carry real operational risk a pure code-review pass could miss if the reviewer only read the .tf diff without checking what Terraform actually intends to do with it.

Merging. Once approved and CI is green, merge; depending on the pipeline design, the actual apply may run automatically post-merge, or require a SEPARATE manual approval gate before applying, distinct from the PR's own code-review approval.

Worked example

The exact commands from the question, extended with the infra-specific CI step made explicit:

git checkout -b feat/update-vpc
# ... edit network/vpc.tf, widening the CIDR block ...
git add .
git commit -m "feat(vpc): increase cidr range"
git push origin feat/update-vpc
# open PR; CI runs:
#   terraform fmt -check
#   terraform validate
#   terraform plan -out=tfplan   (plan output posted to the PR)
# reviewer reads the code diff AND the posted plan output together
# on approval + green CI: merge
# post-merge: terraform apply (auto or gated by a separate approval, per pipeline design)

Trade-offs and pitfalls

• The single biggest practical difference from an application-code change: the CODE DIFF alone does not tell a reviewer what will actually happen. A one-line change to a resource argument can trigger an in-place update OR a destroy-and-recreate depending on that specific argument's ForceNew behavior in the provider; reviewing the diff without reviewing the PLAN is reviewing the WRONG artifact for infrastructure changes specifically.
• Common mistake: treating the plan-output-on-PR step as informational only, not something the reviewer is expected to actually read. If nobody reads the posted plan, the extra CI step provides no real safety benefit over reviewing application code the normal way; the workflow only earns its infra-specific rigor if the plan output is genuinely part of what gets reviewed, not merely generated.
• Merge and apply being separate steps (rather than merge automatically applying) is itself a real design decision with a trade-off, immediate auto-apply on merge keeps the mental model simple (merged means live) but removes a final safety check between "this was approved" and "this is now actually changing production"; a separate, explicit apply gate adds a step but catches the case where the plan changed between review time and merge time (another change merged in between) before it silently applies.
• Common mistake: naming the branch or commit purely after a ticket number with no description of the actual change ("fix/JIRA-4521"); this workflow's whole value depends on Git history being genuinely READABLE later, and a ticket-only name defeats that the moment the ticket system itself becomes hard to search or is retired.

Direct answer

A reusable Terraform module for data infrastructure needs its BOUNDARIES drawn around the resources that genuinely change together as a unit (an S3 bucket plus its own lifecycle policy and encryption config; an EMR/Dataproc cluster plus its own IAM role and security group), with variable design that exposes only what a CONSUMING team actually needs to decide, remote state scoped so a compute cluster's state does not couple to the storage layer's state unnecessarily, and environment-specific differences (sizing, retention, secrets) handled entirely through VARIABLES and per-environment configuration, never through copy-pasted module variants.

Structured elaboration

Module boundaries. Group resources by what actually changes TOGETHER and is meaningfully re-used as a unit: an s3-data-bucket module (bucket, lifecycle policy, encryption, access logging) is one coherent boundary; a spark-cluster module (the EMR/Dataproc cluster itself, its dedicated IAM role, its security group) is another, SEPARATE boundary, even though a given data pipeline consumes both, because a bucket's lifecycle rarely needs to change in lockstep with a cluster's sizing, and coupling them into one module would force every bucket-only change to also touch cluster configuration.

Variable design. Expose the MINIMAL parameter surface a consuming team actually needs to decide (bucket name/prefix, retention period, cluster size, which IAM permissions the cluster needs beyond the module's own sane defaults), keeping everything else (the specific lifecycle-rule structure, the specific encryption algorithm, the specific security-group baseline rules) fixed inside the module so consumers cannot accidentally diverge from the platform's baseline data-governance posture (encryption, retention floors, access logging) the way an unconstrained, fully-parameterized module would allow.

Remote state strategy and locking. Each data-infrastructure COMPONENT (storage, compute) gets its OWN state, following the same per-component isolation principle used generally; a data pipeline's compute layer referencing the storage layer's OUTPUTS (a bucket ARN, for instance) via terraform_remote_state or an explicit variable pass, rather than both living in one combined state file, which would otherwise couple a bucket-retention-policy change's blast radius to the cluster's own apply.

Testing of modules. Module-level integration tests (terratest) provisioning a REAL, ephemeral instance of the module (a genuinely small test bucket, a minimally-sized test cluster) and asserting on its actual configuration (encryption enabled, lifecycle rules present, IAM role scoped correctly), catching a module regression before it reaches any consuming team, run on the module's OWN CI (path-filtered to the module itself), not on every consumer's pipeline.

Handling environment-specific differences and secrets. Environment differences (dev's smaller cluster, prod's stricter retention) are VARIABLE VALUES per environment, never a forked or duplicated module variant; secrets a data pipeline needs (a database credential the cluster reads data from, for instance) are handled through the same referenced-secret pattern used elsewhere, never passed as a raw Terraform variable value that would land in state in plaintext.

Worked example

A concrete module structure for a data pipeline's infrastructure:

modules/
  s3-data-bucket/          # bucket, lifecycle, encryption, access logging
  spark-cluster/           # EMR/Dataproc cluster, its IAM role, security group
pipelines/
  ingestion-pipeline/
    dev/main.tf             # module "bucket" { source = "../../../modules/s3-data-bucket" retention_days = 7 }
                             # module "cluster" { source = "../../../modules/spark-cluster" size = "small" }
    prod/main.tf             # same modules, retention_days = 365, size = "large"

The cluster module's IAM role references the bucket module's OUTPUT (its ARN), passed explicitly as a variable from the consuming pipelines/ingestion-pipeline/*/main.tf (which has access to both modules' outputs within the SAME root configuration), rather than the two modules directly referencing each other, keeping each module independently reusable outside this specific pipeline's combination.

Trade-offs and pitfalls

• Common mistake: bundling storage and compute into ONE module "because this pipeline always uses both together." This couples their state and their change cadence unnecessarily; even when a specific pipeline always pairs them, keeping them as SEPARATE, independently-versioned modules composed together at the pipeline level (as in the worked example) preserves the ability to reuse either one alone in a different context later, and avoids forcing a storage-only change through the compute module's own apply.
• Over-parameterizing a data-infrastructure module (exposing the encryption algorithm, the exact lifecycle-rule shape, as free-form variables) trades platform-team governance for flexibility nobody actually needed; a consuming team should be ABLE to request more permissive settings only through an explicit, reviewed exception, not through an open parameter that quietly lets every consumer set their own baseline.
• Passing a secret value directly as a Terraform variable, even for a "just a connection string" case, lands it in Terraform state in plaintext; a data-pipeline module specifically needs this discipline since data pipelines are exactly the kind of resource that commonly needs database credentials, making this a genuinely common trap for this specific module type.
• Module-level testing costs real money for compute-heavy resources (a test EMR/Dataproc cluster is not free to spin up); sizing the TEST cluster to the smallest viable configuration, and gating the test to run only on changes to the module itself (not on every consuming pipeline's own changes), keeps this cost proportionate.

Direct answer

Immutable digests (image@sha256:...) guarantee that the manifest committed to Git and the bytes actually pulled and run are the SAME image forever, which is exactly what reconciliation, reproducibility, and rollback correctness all depend on; mutable tags (:latest, :v1 reused across builds) let the SAME manifest silently resolve to DIFFERENT bytes at different times, breaking the core GitOps guarantee that Git fully describes what is running. The practical policy: digests (or at minimum strictly immutable, never-reused tags) in every environment a GitOps controller actually reconciles, with a separate, explicit "update the digest" commit as the mechanism for shipping a new version, never implicit re-resolution of a mutable tag; mutable tags are acceptable ONLY in inner-loop developer environments that are explicitly outside the GitOps-reconciled path.

Structured elaboration

Effect on reconciliation. A GitOps controller's reconciliation loop compares the manifest's declared image reference against the cluster's running state. With a digest, "does the running Pod match desired state" is an unambiguous, stable comparison, the digest either matches or it doesn't, and once it matches, reconciliation correctly does nothing further. With a mutable tag, the CONTROLLER sees no drift (the tag string in the manifest hasn't changed), but the ACTUAL running bytes can differ from what was originally deployed if a Pod restarts and re-pulls a tag that has since been overwritten upstream, a form of drift the reconciliation loop is structurally blind to because it only compares the STRING in the manifest, not the resolved content.

Effect on security (CVE, Common Vulnerabilities and Exposures, remediation). A patched image (fixing a CVE) pushed under the SAME mutable tag changes what NEW pods pull without any Git commit recording that change, so there is no audit trail of when the fix actually rolled out, and existing running pods do not automatically pick it up (they keep running the old, pulled bytes until they restart), creating an unpredictable, unrecorded window of mixed-version exposure. A digest-pinned deployment makes CVE remediation an explicit, auditable act: bump the digest in Git, and reconciliation deploys exactly that patched image, deterministically, everywhere, with a Git commit as the permanent record of when the fix went out.

Effect on reproducibility. "Reproducible" means the SAME Git commit always produces the SAME running system. Digests give this property unconditionally. Mutable tags give it only until someone pushes a new image under that tag, at which point the SAME Git commit now resolves differently than it did before, which specifically breaks rollback (reverting to an OLD commit that references :v1 does not guarantee you get back the ORIGINAL :v1 bytes if :v1 has since been overwritten) and breaks any forensic "what was actually running at time T" investigation.

Effect on developer iteration. For an INNER-LOOP dev environment (a developer's own sandbox, rapidly rebuilding and testing), requiring a fresh digest and a Git commit for every single iteration is genuine friction that does not serve any of the guarantees above, since nobody is trying to prove reproducibility or audit a dev sandbox's history the way they would a production deployment.

Worked example

A recommended tiered policy:

The transition from a build artifact to a production digest reference should be an EXPLICIT step in the pipeline (CI resolves the newly built image's digest and commits a manifest update referencing it), not something a human copies by hand, since a manually-typed digest is exactly the kind of error-prone step this policy exists to eliminate.

Trade-offs and pitfalls

• Common mistake: using a "unique-looking" tag (a commit SHA or build number) and assuming it is equivalent to a digest. It usually is, in practice, AS LONG AS the registry and CI pipeline genuinely never reuse it; but this is a PROCESS guarantee (nobody re-pushes that tag), not a CRYPTOGRAPHIC one the way a digest is (the digest IS the content hash, so it cannot silently point to different bytes by definition). A digest removes the reliance on that process discipline entirely.
• Common mistake: enforcing digest-only policy in production manifests but leaving the ADMISSION path open to mutable tags, so a manifest applied outside the normal GitOps flow (a manual kubectl apply, a different pipeline) can still introduce a mutable-tag image; an admission-controller policy (Kyverno/Gatekeeper) rejecting any Pod spec without a resolved digest closes this gap regardless of HOW the manifest arrived.
• Digests make manifests harder for a human to read at a glance (sha256:a1b2c3... conveys no version information the way :v2.3.1 does); a common, workable mitigation is keeping a human-readable tag in the commit message or a companion annotation/label while the actual image: field uses the digest, giving both machine-verifiable immutability and human-readable context.
• Rollback via Git revert only fully works if EVERY commit in history referenced digests, not mutable tags. A rollback to a historical commit that referenced :v1 is not a true rollback if :v1 has since been overwritten; digest pinning is what makes "revert the commit" and "actually get back the old running state" the same operation.

Direct answer

Terraform state snapshots, PKI (public key infrastructure) binaries, and large AMI (Amazon Machine Image) artifacts share one property that makes storing them directly in Git a bad fit: Git's storage model is optimized for TEXT that diffs well and history that stays small, while these are large, frequently-regenerated, mostly-BINARY blobs that Git would store as a full new copy on every change, bloating every clone forever. The right home depends on what the artifact actually needs: Git LFS (Large File Storage) for files that genuinely need Git-native versioning and PR-based review despite being binary; a dedicated artifact repository (Artifactory, or plain object storage like S3) for anything that is really a BUILD OUTPUT with its own release lifecycle; and Terraform's own remote backend, never Git at all, for state specifically, since state's locking, current-value, and sensitive-data properties do not map onto Git's model at all.

Structured elaboration

Git LFS. Stores large files as pointers in Git (small text references) while the actual bytes live in a separate LFS-aware storage backend; Git operations (clone, diff, history) stay fast because the large content is fetched separately and only on demand. This fits files that genuinely benefit from being tied to a specific commit and reviewed via a normal PR flow (a PKI root certificate bundle that changes rarely and where "which commit introduced this exact cert" matters), but LFS storage and bandwidth typically cost more per gigabyte than plain object storage, and LFS is a worse fit for anything that changes FREQUENTLY (every LFS-tracked version is still a distinct stored blob, so frequent large-file churn is expensive regardless of the pointer trick).

Artifact repositories (Artifactory, S3 as a plain artifact store). Purpose-built for exactly this: large binary build outputs, versioned, with lifecycle policies (retention, promotion between repositories for dev/staging/prod), typically far cheaper per gigabyte than Git LFS, and with retrieval APIs designed for CI/CD consumption (pull the specific version a pipeline needs, not the whole history). This is the right home for large AMI artifacts specifically, an AMI has its OWN natural versioning and promotion lifecycle (built once, promoted through environments, eventually deprecated) that maps directly onto an artifact repository's model, and gains nothing from Git's commit-and-diff semantics since an AMI is not something anyone reviews as a text diff.

Remote backends, for Terraform state specifically. State is fundamentally different from the other two categories: it needs LOCKING (to prevent concurrent-write corruption), it represents CURRENT truth (not a historical build artifact with old versions kept around for their own sake), and it routinely contains SENSITIVE VALUES. None of Git, Git LFS, or a generic artifact repository provide locking as a first-class concept; Terraform's own remote backends (S3+DynamoDB, Terraform Cloud) are purpose-built for exactly this and are the only appropriate home, state should never be committed to Git in any form, LFS-tracked or otherwise.

Worked example

A concrete assignment for a mid-size infrastructure org:

Integration into PR reviews, CI, and access controls. For the LFS-tracked PKI bundles, a PR reviewer sees the LFS pointer diff and, critically, needs LFS-aware tooling to actually inspect the real content change (a plain git diff on an LFS pointer shows only the pointer hash changing, not the certificate content itself, so the review process needs an explicit step, a CI job that renders and posts the actual diff, to make review meaningful rather than rubber-stamping a hash change). For AMI artifacts in the artifact repository, CI publishes a new version on build and the PROMOTION step (dev to staging to prod) is a separate, reviewed action referencing that specific version by its immutable identifier. Access control differs meaningfully by artifact type: PKI material needs the tightest read scoping of anything on this list (arguably tighter than most application secrets), while AMI artifacts typically need broad READ access (anything provisioning from them) but narrow WRITE access (only the build pipeline that produces them).

Trade-offs and pitfalls

• Common mistake: committing Terraform state, even encrypted, to Git or Git LFS "for convenience," because it is technically just a file. This is a recurring, real mistake distinct from the general large-artifact question; even encrypted, Git provides no locking, so this reintroduces the exact concurrent-write-corruption risk remote backends specifically exist to solve, an LFS-stored state file is still Git-adjacent enough to tempt someone into a normal git commit workflow for it, defeating the whole point.
• Git LFS's per-gigabyte cost, multiplied by CHANGE FREQUENCY, is the single most common reason a team regrets choosing it for something that turned out to churn more than expected. A file that seemed like a natural LFS candidate at adoption time (infrequent changes) can quietly become expensive if its actual change cadence increases later; periodically reviewing what is tracked in LFS against its actual current churn rate catches this before the cost surprise does.
• A plain git diff on an LFS pointer is actively misleading if reviewers are not aware LFS is in use, it shows a hash changing, which LOOKS like a trivial change and can get rubber-stamped, while hiding a potentially significant underlying content change; any team adopting LFS needs an explicit review-tooling step (or at minimum, clear reviewer training) to close this gap.
• Artifact repository retention/lifecycle policies need to reconcile with any compliance-driven minimum-retention requirement: cost-optimized automatic cleanup can silently violate a longer regulatory retention window if the two policies are not designed together.