Repository navigation
Conversation
Makefile.terraform's frontend-only target cd'd into the environment directory and ran `terraform apply -target=module.frontend` itself, so it skipped everything tf-deploy.sh does around the apply: the profile existence check, the environment-directory bootstrap, the automatic `terraform init`, the AWS_PROFILE warning and the outputs summary. It also kept its own copy of the ../../../profiles path arithmetic. tf-deploy.sh forwards extra arguments to terraform verbatim, so the target now calls `./scripts/tf-deploy.sh $(PROVIDER) $(PROFILE) apply -target=module.frontend`, like deploy/plan/destroy/docker-skip already do. The apply still has no -auto-approve, so terraform still asks before changing anything. scripts/test-tf-deploy.sh runs the targets and the script in a temp copy of the layout with a stub terraform that only records its argv, and fails on the old recipe (13 of 14 checks). A new CI job runs it. Closes LeanerCloud#59 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
Makefile.terraform'sfrontend-onlytarget was the only state-changing target that didn't go throughscripts/tf-deploy.sh. It ran terraform itself:Because of that it skipped everything the script does around the apply: the profile existence check, the environment-directory bootstrap (mkdir plus the
main.tfsymlink), the automaticterraform initwhen.terraformis missing, theAWS_PROFILEwarning and the "Deployment Outputs" summary. It also kept its own copy of the../../../profiles/...path arithmetic.Fix
tf-deploy.shalready passes extra arguments to terraform unchanged, so the target now reads:This matches how
deploy,plan,destroy,outputanddocker-skipalready work. The apply still has no-auto-approve, so terraform still asks before changing anything, as it did before.terraform argv with the env dir present (recorded by a stub
terraform):apply -var-file=../../../profiles/aws/dev.tfvars -target=module.frontend(no init, even when.terraformis missing)init, thenapply -var-file=<root>/terraform/profiles/aws/dev.tfvars -target=module.frontend, thenoutputapply -var-file=<root>/terraform/profiles/aws/dev.tfvars -target=module.frontend, thenoutputProfile not found: ...(before:apply -var-file=../../../profiles/aws/nope.tfvars ...reached terraform)main.tfsymlink (before:cd: can't cd to terraform/environments/aws/dev)Regression test
scripts/test-tf-deploy.sh(new; the repo had no test fortf-deploy.sh) copiesMakefile.terraformandscripts/tf-deploy.shinto amktemp -dtree laid out like the repo. It puts a stubterraformon PATH that only appends its argv to a log, then checks:make -n frontend-onlyprints./scripts/tf-deploy.sh aws dev apply -target=module.frontendand no directterraform apply.main.tfsymlink get created, the calls are exactlyinit/ targetedapply/output, and the output includes the outputs summary and theAWS_PROFILEwarning.initstill runs first.initis skipped.Profile not found, and terraform is never called.Makefile.terraformrecipe runsterraform apply|plan|destroy|refresh|importdirectly.A new
tf-deploy-wrapperjob inci.ymlruns it (shell only,contents: read, same shape asmigration-down-gate), andci-successnow depends on it.Closes #59
Verification
All of this is local stub and fixture evidence. Nothing ran terraform against a real backend.
Makefile.terraform:bash scripts/test-tf-deploy.shgaveResults: 1 passed, 13 failed(only check 6 passed, since it covers the script alone).bash scripts/test-tf-deploy.shgaveResults: 14 passed, 0 failed.make -n -f Makefile.terraform frontend-only PROVIDER=aws PROFILE=devprints:shellcheck scripts/test-tf-deploy.sh(0.11.0): clean.scripts/tf-deploy.shis unchanged; it has pre-existing SC2011/SC2162/SC2181 findings that this PR leaves alone.actionlint .github/workflows/ci.yml: clean.pre-commit run --files Makefile.terraform scripts/test-tf-deploy.sh .github/workflows/ci.yml: all applicable hooks passed (including actionlint and zizmor), exceptgit-secretsandtrivy-config, which couldn't run because those binaries aren't installed locally.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
Makefile.terraformtargets still call terraform directly, but none of them applies or plans:validate(terraform validate),fmt(terraform fmt -recursive),state-list(terraform state list) andstate-show(terraform state show $(RESOURCE)). The issue only coversfrontend-only, and routing these through the script wouldn't work as written: its catch-all branch adds-var-file=...straight after the action word (e.g.terraform state -var-file=... list), whichstatesubcommands reject. They're left as they were.docs/audits/codebase-audit-2026-09-02.mdnotes,tf-deploy.shusesterraform/environments/<provider>/<profile>as the working directory, but the repo has one flat root per provider (terraform/environments/aws), andterraform/profiles/aws/holds only*.tfvars.example. So the local script path, and with this PRmake frontend-onlytoo, stops at the profile check until someone creates a profile. That's a problem with the script's layout, not with this target. Routing through the script means a future fix to the layout coversfrontend-onlyautomatically, instead of needing a second copy of the path logic.ci.yml(it adds a step to theaws-tfstate-platform-keyjob). The hunks don't overlap.Maintainer: please mirror the issue's triage labels onto this PR:
type/chore,impact/internal,priority/p3,severity/low,urgency/eventually,effort/xs,triaged.🤖 Generated with Claude Code