Skip to content

fix(auto-approve-bot-prs): merge directly and surface skips that need action - #208

Open
sydorovdmytro wants to merge 1 commit into
mainfrom
devops-1209/direct-bot-merge
Open

fix(auto-approve-bot-prs): merge directly and surface skips that need action#208
sydorovdmytro wants to merge 1 commit into
mainfrom
devops-1209/direct-bot-merge

Conversation

@sydorovdmytro

Copy link
Copy Markdown
Contributor

Follow-up to #201 (the in-pipeline pro dependency bump). Three changes, all aimed at the same failure class: the action was silent about outcomes that require someone to act, so a PR that never merged looked identical to a PR that merged fine.

References DEVOPS-1209

Merge directly, keep --auto as the fallback

By the time the merge step runs, wait-for-ci.sh has confirmed every other check is green and the PR is approved, so there is normally nothing left for GitHub's auto-merge queue to wait on. --auto additionally requires the repository's allow_auto_merge setting, which is invisible from inside the action and silently turns the merge into a no-op when it is off — and a merge that never happens strands whatever is waiting on it. The vcluster-release orchestrator blocks on exactly this merge during a legacy release cut, so that no-op stalls a release.

A refused direct merge still degrades to queueing, which is the right answer when a required check registered after the CI wait declared green. Re-runs are handled explicitly: an already-merged PR is benign, and a PR closed unmerged is a human decision, so neither escalates.

Report actionable skips at warning/error level

Nothing here exits non-zero — the composite must not red-X a caller's CI over an unrelated bot PR — but notices are invisible on a green run, and the cause of a non-merge is only knowable inside the action. A caller waiting on the merge can see that it did not happen, never why.

An approved-but-unmerged PR now carries both underlying gh errors in an ::error:: annotation. Four causes that previously shared one notice are split, because they need different responses:

cause before now
merge conflict with base ::notice::PR mergeability is '…' ::warning:: — needs a rebase
mergeability budget exhausted same notice ::warning:: — transient, re-run
token cannot resolve its own user ::notice::Skipping approval (…) ::error:: — expired/insufficient scope
approver identity == PR author same notice ::error:: — self-approval is impossible
approved but not merged, not queued ::notice::gh pr merge failed ::error:: with both gh errors

A real defect found while splitting those

--jq '.mergeable // "null"' collapsed a genuine mergeable: false into "null", because jq's // treats false as an empty value:

$ echo '{"mergeable":false}' | jq -r '.mergeable // "null"'
null

So a conflicted PR was indistinguishable from un-computed metadata, and burned the full ~30s retry budget re-polling a value that was already definitive. The filter now tests for null explicitly. Both halves are pinned by regression tests that fail against the old filter and pass with the fix:

not ok 2 mergeable=false → proceed=false, reported as a conflict needing a rebase
not ok 3 regression: mergeable=false is definitive and must not burn the retry budget

Tests

enable-auto-merge.sh had no coverage; this adds enable-auto-merge.bats (10 cases: direct-merge success without touching --auto, fallback-to-queue, both-refused escalation, already-merged, closed-unmerged, unreadable state, invalid merge method, per-method passthrough, env guards). The gh mock gains separate control of the direct and --auto merges plus gh pr view --json state.

make test-auto-approve-bot-prs — 61 pass, 0 fail. shellcheck, actionlint and make check-docs clean.

Compatibility

No new inputs; auto-merge: true behaves the same from a caller's point of view, minus the dependency on the repo setting. Existing callers on auto-merge: false are unaffected by the merge change and only see the improved annotations. Docs regenerated via make generate-docs.

… action

Three changes, all aimed at the same failure class: the action was silent about
outcomes that require someone to act, so a PR that never merged looked
identical to a PR that merged fine.

Merge directly, keep --auto as the fallback. By the time the merge step runs,
wait-for-ci.sh has confirmed every other check is green and the PR is approved,
so there is normally nothing left for GitHub's auto-merge queue to wait on.
`--auto` additionally requires the repository's allow_auto_merge setting, which
is invisible from here and silently turns the merge into a no-op when it is off
- and a merge that never happens strands whatever is waiting on it (the
vcluster-release orchestrator blocks on exactly this merge during a legacy
release cut). A refused direct merge still degrades to queueing, which is the
right answer when a required check registered after the CI wait declared green.
Re-runs are handled explicitly: an already-merged PR is benign and a PR closed
unmerged is a human decision, so neither escalates.

Report actionable skips at warning/error level. Nothing here exits non-zero -
the composite must not red-X a caller's CI over an unrelated bot PR - but
notices are invisible on a green run, and the cause of a non-merge is only
knowable here (a caller waiting on the merge can see that it did not happen,
never why). An approved-but-unmerged PR now carries both underlying gh errors
in an ::error:: annotation. Conflicts, exhausted mergeability budgets, dead
tokens and same-identity tokens are split apart instead of sharing one notice,
since they need different responses.

Fix a real defect found while splitting those: `--jq '.mergeable // "null"'`
collapsed a genuine `mergeable: false` into "null", because jq's `//` treats
false as empty. A conflicted PR was therefore indistinguishable from
un-computed metadata AND burned the full ~30s retry budget re-polling a value
that was already definitive. Both halves are pinned by regression tests that
fail against the old filter.

Adds enable-auto-merge.bats (the script had no coverage) and extends the gh
mock to control the direct and --auto merges separately and to answer
`gh pr view --json state`. 61 tests pass; shellcheck and actionlint clean.
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.

1 participant