Skip to content

Commit b1670c3

Browse files
committed
ci: merge after approval, not auto-merge on green
main is governed by a ruleset, not classic branch protection — which is why my earlier check of branches/main/protection returned 404 and I wrongly concluded the branch was unprotected. The ruleset has required_approving_review_count: 1, require_code_owner_review: true and zero bypass actors, and .github/CODEOWNERS assigns `*` to @aws/aws-lambda-tooling. Every Dependabot pull request is therefore BLOCKED with reviewDecision=REVIEW_REQUIRED: #842 BLOCKED REVIEW_REQUIRED MERGEABLE #841 BLOCKED REVIEW_REQUIRED MERGEABLE #839 BLOCKED REVIEW_REQUIRED MERGEABLE So the workflow could never have merged anything, and worse, it hid that: BLOCKED was lumped in with conflicts and reported as "most likely a sibling update landed first", so the hourly sweep would have skipped every pull request forever with a diagnosis that was simply wrong. Three changes, no governance change. No bypass actor is added and no approval is forged: a bot approval cannot satisfy a code-owner requirement anyway, and whether CI should be allowed to merge without review is the code owners' call, not this branch's. * reviewDecision is now a gate, checked last so that reaching it means the pull request is example-only, verified at its current head, and green. A pull request waiting on review is reported as exactly that, which turns the sweep's job summary into a worklist of "verified, waiting only on you". * BLOCKED is classified separately from DIRTY/BEHIND/DRAFT/UNKNOWN and names the ruleset as the cause. * The workflow is renamed Dependabot Merge and its header, the script's header and the pull request description say merge-after-approval rather than auto-merge. The script's own rollup exclusion is updated to match the new name. Twelve paths exercised: the four reviewDecision states, the three post-merge failure classifications under an approval, and the five earlier gates still firing ahead of the review check.
1 parent 8a35cb2 commit b1670c3

2 files changed

Lines changed: 49 additions & 10 deletions

File tree

.github/scripts/dependabot-automerge.sh

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
#!/usr/bin/env bash
22
#
3-
# Merges one Dependabot pull request, if it is an example-only update that Verify
4-
# Examples has verified at the pull request's current head.
3+
# Merges one Dependabot pull request, if it is an example-only update that a code owner
4+
# has approved and that Verify Examples has verified at the pull request's current head.
5+
#
6+
# Not auto-merge-on-green: main is governed by a ruleset requiring one code-owner approval
7+
# with zero bypass actors, so nothing can merge without a human. What this removes is the
8+
# second trip — approve once and the merge happens within the hour, but only if the
9+
# verification covers the exact commit being merged, so a stale approval cannot land an
10+
# unverified head.
511
#
612
# Usage: REPO=<owner/repo> dependabot-automerge.sh <pr-number>
713
#
@@ -53,7 +59,7 @@ skip() {
5359
# result is not wrapped: unlike the request, it cannot fail transiently, and a parse
5460
# failure there is a real fault that should be loud.
5561
if ! pr_json=$(gh pr view "$PR" --repo "$REPO" \
56-
--json author,headRefOid,state,statusCheckRollup); then
62+
--json author,headRefOid,state,reviewDecision,statusCheckRollup); then
5763
skip "could not read the pull request."
5864
fi
5965

@@ -96,7 +102,7 @@ fi
96102

97103
not_green=$(jq -r '
98104
.statusCheckRollup[]?
99-
| select((.workflowName // "") != "Dependabot Auto-merge")
105+
| select((.workflowName // "") != "Dependabot Merge")
100106
| select([((.conclusion // .state // "PENDING") | ascii_upcase)]
101107
- ["SUCCESS", "SKIPPED", "NEUTRAL"] | length > 0)
102108
| ((.name // .context) + " = " + (.conclusion // .state // "PENDING"))' <<<"$pr_json")
@@ -158,7 +164,28 @@ if [[ -n "$unverified" ]]; then
158164
skip "in the matrix but not verified by run $run_id: $(join_list "$unverified")"
159165
fi
160166

161-
echo "PR #$PR is example-only and verified at $head_sha by run $run_id. Merging."
167+
# The approval is the last gate, and it is checked here rather than earlier on purpose:
168+
# reaching this line means the pull request is example-only, verified at its current head,
169+
# and green. Reporting it now makes the job summary a worklist of "verified, waiting only
170+
# on you" rather than a list of things that may also be unverified.
171+
#
172+
# main is governed by a ruleset (not classic branch protection, which is why
173+
# `branches/main/protection` returns 404): one approving review, `require_code_owner_review`,
174+
# and zero bypass actors, with .github/CODEOWNERS assigning `*` to @aws/aws-lambda-tooling.
175+
# No token can merge past that and no bot approval can satisfy it, so this workflow merges
176+
# after a human approves — it does not approve on anyone's behalf.
177+
review=$(jq -r '.reviewDecision // ""' <<<"$pr_json")
178+
case "$review" in
179+
APPROVED) ;;
180+
CHANGES_REQUESTED)
181+
skip "verified at ${head_sha:0:8} by run $run_id, but a reviewer requested changes."
182+
;;
183+
*)
184+
skip "verified at ${head_sha:0:8} by run $run_id — waiting for a code-owner approval (@aws/aws-lambda-tooling)."
185+
;;
186+
esac
187+
188+
echo "PR #$PR is example-only, verified at $head_sha by run $run_id, and approved. Merging."
162189

163190
# --match-head-commit closes the remaining window: if the branch moves between the
164191
# lookups above and this call, the API rejects the merge rather than applying it to an
@@ -190,7 +217,13 @@ if [[ "$head_after" != "$head_sha" ]]; then
190217
skip "merge rejected, head moved to $head_after."
191218
fi
192219
case "$state" in
193-
DIRTY | BLOCKED | BEHIND | DRAFT | UNKNOWN)
220+
# BLOCKED is called out separately because it used to be lumped in with conflicts and
221+
# reported as "a sibling update landed first", which was simply the wrong diagnosis:
222+
# main's ruleset blocks a merge until the required review is satisfied.
223+
BLOCKED)
224+
skip "merge rejected, blocked by main's ruleset (review or a required check) — reviewDecision was $review."
225+
;;
226+
DIRTY | BEHIND | DRAFT | UNKNOWN)
194227
skip "merge rejected, not mergeable (mergeStateStatus=$state) — most likely a sibling update landed first."
195228
;;
196229
esac

.github/workflows/dependabot-automerge.yaml

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
1-
name: Dependabot Auto-merge
1+
name: Dependabot Merge
22

3-
# Merges Dependabot pull requests that only touch example applications, once Verify
4-
# Examples has actually verified them. The decision lives in
3+
# Merges Dependabot pull requests that only touch example applications, once a code owner
4+
# has approved them and Verify Examples has verified them.
5+
#
6+
# Deliberately not "auto-merge on green": main is governed by a ruleset with one required
7+
# code-owner approval and zero bypass actors, so no token here can merge without a human,
8+
# and a bot approval cannot satisfy a CODEOWNERS requirement. The value is in the other
9+
# half — an approval no longer means "and come back later to click merge", and the merge
10+
# only happens if the verification covers the exact commit that lands. The decision lives in
511
# .github/scripts/dependabot-automerge.sh; this file only decides which pull requests to
612
# offer it.
713
#
@@ -42,7 +48,7 @@ on:
4248
# mergeStateStatus branch in dependabot-automerge.sh turn the loser into a skip with a
4349
# reason rather than a red run.
4450
concurrency:
45-
group: dependabot-automerge-${{ github.event.workflow_run.pull_requests[0].number || 'sweep' }}
51+
group: dependabot-merge-${{ github.event.workflow_run.pull_requests[0].number || 'sweep' }}
4652
cancel-in-progress: false
4753

4854
permissions:

0 commit comments

Comments
 (0)