Skip to content

fix(apply-terraform-plan): don't fail a green apply on cleanup-PR merge - #330

Open
jlauzy wants to merge 1 commit into
mainfrom
apply-terraform-plan-cleanup-merge-nonfatal
Open

fix(apply-terraform-plan): don't fail a green apply on cleanup-PR merge#330
jlauzy wants to merge 1 commit into
mainfrom
apply-terraform-plan-cleanup-merge-nonfatal

Conversation

@jlauzy

@jlauzy jlauzy commented Aug 7, 2026

Copy link
Copy Markdown
Member

Problem

After a successful terraform apply, the action opens a plan-file cleanup PR and merges it with the GITHUB_TOKEN, retrying five times before exit 1. On a repo whose base branch requires an approving review, that merge can never succeed — the token acts as the actions bot, and a bot cannot approve its own PR:

GraphQL: At least 1 approving review is required by reviewers with write access. (mergePullRequest)
X Pull request Khan/khan-monitoring#40 is not mergeable: the base branch policy prohibits the merge.
Merge attempt 5 failed; retrying in 5s...
Failed to merge cleanup PR #40 after several attempts
##[error]Process completed with exit code 1

The apply had already succeeded in that same job:

Apply complete! Resources: 0 added, 4 changed, 0 destroyed.

So the job reports a completed deploy as a failed one, and the real signal is buried under a cleanup failure. The 2.2.4 release that introduced the cleanup PR assumed a base branch that "requires PRs but no approvals or required status checks" — repos that also require a review have never had a green apply. In Khan/khan-monitoring this is 5 of its 6 apply slos failures, and a human has had to merge every cleanup PR by hand.

It also cascades: that release's other assumption — "a GITHUB_TOKEN merge doesn't trigger further push workflows" — silently stops holding, because the human merge that unblocks things does re-trigger the apply workflow, which then hits Error: Saved plan is stale on a sibling stack.

Fix

The retry loop now falls back to enabling auto-merge (a no-op where auto-merge is disabled) and emits a ::warning:: with the PR URL instead of exiting non-zero. An unmerged cleanup PR only leaves a stale plan pointer on the base branch, which the next run's staleness check already catches. A failing terraform apply still fails the job — the trailing Mark job as failed step keys on steps.apply.outcome, unchanged.

The one non-obvious bit

Delete uploaded plan from GCS was gated only on reaching it, which was safe when an unmergeable cleanup PR failed the job. Now that it doesn't, "we got here" no longer implies "the pointer is gone from the base branch" — so the step is gated on the merge step's new merged output. This preserves the 3.0.0 invariant: delete the uploaded plan only once the cleanup PR has actually landed, or the base branch keeps a pointer to a missing object and every re-run hard-fails at download.

Alternatives considered

Fixing this consumer-side, in khan-monitoring, was worse in every variant: a ruleset bypass for the actions app grants the bot blanket push-bypass on main rather than just its own cleanup PRs; continue-on-error on the uses: step masks non-apply failures (apply_failed is false when apply never ran, e.g. GCP auth or plan download); and an auto-approve workflow for ci/terraform-cleanup-* races this action's ~25s retry loop.

Patch-level: no input surface changes, and the new behavior only replaces a path that previously always ended in exit 1. All four other consumers (beep-boop, culture-cron, internal-services, internal-webserver) are on v3.0.0 and merge their cleanup PRs successfully today, so they never enter this path.

Testing

action.yml parses; utils/terraform-plan-object-key.test.ts is untouched (no object-key changes). The end-to-end proof is khan-monitoring's next apply once it is migrated to v3 — it is the only consumer still on v2 and the only one with a review-requiring base branch, so it's the repo that reproduces this.

🤖 Generated with Claude Code

The action opens a plan-file cleanup PR after a successful apply and merges
it with the GITHUB_TOKEN, retrying 5x before exit 1. On a base branch that
requires an approving review that merge can never succeed — a bot cannot
approve its own PR — so the job goes red on a deploy that completed fine.

Fall back to auto-merge and warn instead of exiting non-zero. Gate the
GCS plan deletion on the merge step's new `merged` output, since reaching
that step no longer implies the pointer left the base branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 761b1c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
apply-terraform-plan Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khan-actions-bot
khan-actions-bot requested review from a team, jeresig and somewhatabstract and removed request for a team August 7, 2026 17:55

@jeresig jeresig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants