Repository navigation
feat(source-control): refuse a held or superseded draft in pull-request ready #7345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e401017
feat(source-control): refuse a held or superseded draft in pull-reque…
kyle-sexton 2b5591e
fix(source-control): escape the typographic apostrophe in the hold check
kyle-sexton 104054b
fix(source-control): close the hold check's fail-open paths
kyle-sexton 29bc083
fix(source-control): recheck holds right before the ready flip
kyle-sexton 7dfa794
fix(source-control): trim every leading space from a quoted hold line
kyle-sexton 9d1be22
Merge remote-tracking branch 'origin/main' into feat/pr-ready-hold-check
kyle-sexton 85f9959
fix(source-control): let a Hold: line through the ready flip
kyle-sexton c83d26e
test(source-control): fix a spelling the typos check rejects
kyle-sexton 89bdef8
fix(source-control): run the hold recheck at the flip step
kyle-sexton 86a0bdc
docs(source-control): say the ready hold check shares only the do-not…
kyle-sexton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| bump: minor | ||
| --- | ||
|
|
||
| ### Changed | ||
|
|
||
| - `/source-control:pull-request ready` leaves a held or superseded draft in draft and reports why: a `do-not-merge` label, a body that says "do not merge", a WIP title marker, or an already-closed linked issue. The check is `check-ready-holds.sh`, which fails closed when it cannot read the pull request. |
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
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
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
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
84 changes: 84 additions & 0 deletions
84
plugins/source-control/skills/pull-request/scripts/check-ready-holds.sh
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| #!/usr/bin/env bash | ||
| # check-ready-holds.sh — Refuse the ready flip for a held or superseded pull request. | ||
| # | ||
| # The `ready` action runs this before it touches the branch. A pull request is | ||
| # held when it carries the do-not-merge label (any case), its body says "do not | ||
| # merge" or "don't merge", or its title carries WIP as a word. A body line | ||
| # starting "Hold:" holds the merge only, so it does not hold the flip. It is | ||
| # superseded when an issue it closes is already closed. | ||
| # | ||
| # Usage: | ||
| # check-ready-holds.sh <pr> [--repo <owner/repo>] | ||
| # | ||
| # Exit codes: | ||
| # 0 no hold: the flip may go on | ||
| # 1 invalid argument | ||
| # 2 a gh call failed; the flip stops, since no hold was ruled out | ||
| # 3 held or superseded: one reason per line on stdout | ||
| # 5 prerequisite missing (gh, jq) | ||
|
|
||
| # portability-scope: the GitHub implementation of the ready flip's hold check; every read is a gh call. | ||
| set -uo pipefail | ||
|
|
||
| usage() { | ||
| sed -n '2,/^$/p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//' | ||
| exit 0 | ||
| } | ||
|
|
||
| die() { | ||
| printf 'check-ready-holds: %s\n' "$2" >&2 | ||
| exit "$1" | ||
| } | ||
|
|
||
| PR="" REPO="" | ||
| while (($# > 0)); do | ||
| case "$1" in | ||
| -h | --help) usage ;; | ||
| --repo) | ||
| (($# >= 2)) || die 1 "--repo needs a value" | ||
| REPO="$2" | ||
| shift 2 | ||
| ;; | ||
| -*) die 1 "unknown flag $(printf '%q' "$1") (use --help)" ;; | ||
| *) | ||
| [[ -z "$PR" ]] || die 1 "unexpected argument $(printf '%q' "$1")" | ||
| PR="$1" | ||
| shift | ||
| ;; | ||
| esac | ||
| done | ||
| [[ "$PR" =~ ^[0-9]+$ ]] || die 1 "a pull request number is required (use --help)" | ||
|
|
||
| command -v gh >/dev/null 2>&1 || die 5 "gh not found on PATH" | ||
| command -v jq >/dev/null 2>&1 || die 5 "jq not found on PATH" | ||
|
|
||
| repo_args=() | ||
| [[ -n "$REPO" ]] && repo_args=(--repo "$REPO") | ||
|
|
||
| facts=$(gh pr view "$PR" ${repo_args[@]+"${repo_args[@]}"} --json title,body,labels,closingIssuesReferences) || | ||
| die 2 "gh pr view $PR failed" | ||
|
|
||
| # The body pattern is the do-not-merge alternative of babysit-prs' | ||
| # BODY_MERGE_HOLD_RE (babysit_merge.py); its `Hold:` line alternative holds the | ||
| # merge only, so it is left out here. The hyphenated label name is not matched, | ||
| # so a body that only names the label holds nothing. | ||
|
kyle-sexton marked this conversation as resolved.
|
||
| reasons=$(jq -r ' | ||
| ([.labels[]?.name | select(ascii_downcase == "do-not-merge")] | if length > 0 then "label: the pull request carries do-not-merge" else empty end), | ||
| (.body // "" | if test("\\bdo(?:n['"'"'\u2019]?t| not)\\s+merge\\b"; "i") then "body: it says do not merge" else empty end), | ||
| (.title // "" | if test("\\bwip\\b"; "i") then "title: a WIP marker in \"\(gsub("[[:cntrl:]]"; ""))\"" else empty end) | ||
| ' <<<"$facts") || die 2 "cannot parse the pull request facts" | ||
|
|
||
| refs=$(jq -r '.closingIssuesReferences[]? | [(.url | capture("^https?://(?<h>[^/]+)/").h), "\(.repository.owner.login)/\(.repository.name)", (.number | tostring)] | @tsv' <<<"$facts") || | ||
| die 2 "cannot parse the closing issue references" | ||
|
|
||
| while IFS=$'\t' read -r host path num; do | ||
| [[ -n "$num" ]] || continue | ||
| # REST, not GraphQL: some sandboxed sessions are refused GraphQL. | ||
| state=$(gh api --hostname "$host" "repos/$path/issues/$num" --jq .state | tr -d '\r') || | ||
| die 2 "cannot read the state of $path#$num" | ||
| [[ "$state" == closed ]] && reasons+="${reasons:+$'\n'}superseded: it closes $path#$num, which is already closed" | ||
| done <<<"$refs" | ||
|
|
||
| [[ -z "$reasons" ]] && exit 0 | ||
| printf '%s\n' "$reasons" | ||
| exit 3 | ||
139 changes: 139 additions & 0 deletions
139
plugins/source-control/skills/pull-request/scripts/check-ready-holds.test.sh
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| #!/usr/bin/env bash | ||
| # Regression tests for check-ready-holds.sh, black-box with a stubbed `gh` on PATH. | ||
|
|
||
| set -uo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| SCRIPT="$SCRIPT_DIR/check-ready-holds.sh" | ||
| TEST_TMPDIR="$(mktemp -d)" | ||
| trap 'rm -rf "$TEST_TMPDIR"' EXIT | ||
|
|
||
| # shellcheck source=../../../scripts/test-helpers.sh | ||
| source "$SCRIPT_DIR/../../../scripts/test-helpers.sh" | ||
|
|
||
| command -v jq >/dev/null 2>&1 || skip_suite "jq not installed" | ||
|
|
||
| STUB_DIR="$TEST_TMPDIR/stubs" | ||
| mkdir -p "$STUB_DIR" | ||
| # The pull request's facts come from $TEST_TMPDIR/pr.json; an issue's state | ||
| # from $TEST_TMPDIR/issue-<n>, and a missing file fails the lookup. | ||
| cat >"$STUB_DIR/gh" <<STUB_EOF | ||
| #!/usr/bin/env bash | ||
| printf '%s\n' "\$*" >>"$TEST_TMPDIR/calls" | ||
| case "\$1 \$2" in | ||
| "pr view") [[ -f "$TEST_TMPDIR/pr.json" ]] || exit 1; cat "$TEST_TMPDIR/pr.json" ;; | ||
| "api "*) | ||
| for a in "\$@"; do [[ "\$a" == repos/* ]] && n="\${a##*/}"; done | ||
| [[ -f "$TEST_TMPDIR/issue-\$n" ]] || exit 1 | ||
| cat "$TEST_TMPDIR/issue-\$n" | ||
| ;; | ||
| *) echo "gh-stub: unrouted \$*" >&2; exit 1 ;; | ||
| esac | ||
| STUB_EOF | ||
| chmod +x "$STUB_DIR/gh" | ||
|
|
||
| # pr <title> <body> [labels-json] [closing-refs-json] | ||
| pr() { | ||
| jq -n --arg t "$1" --arg b "$2" --argjson l "${3:-[]}" --argjson c "${4:-[]}" \ | ||
| '{title: $t, body: $b, labels: $l, closingIssuesReferences: $c}' >"$TEST_TMPDIR/pr.json" | ||
| } | ||
|
|
||
| ref() { | ||
| printf '{"number":%s,"url":"https://github.com/acme/app/issues/%s","repository":{"name":"app","owner":{"login":"acme"}}}' "$1" "$1" | ||
| } | ||
|
|
||
| run() { | ||
| : >"$TEST_TMPDIR/calls" | ||
| PATH="$STUB_DIR:$PATH" bash "$SCRIPT" "$@" | ||
| } | ||
|
|
||
| pr "feat(x): add a thing" "Closes #10" "[]" "[$(ref 10)]" | ||
| echo open >"$TEST_TMPDIR/issue-10" | ||
| out=$(run 7) | ||
| assert_exit "an unheld pull request with an open linked issue exits 0" 0 "$?" | ||
| assert_silent "an unheld pull request prints nothing" "$out" | ||
| assert_contains "the issue state is read over REST" "$(cat "$TEST_TMPDIR/calls")" "repos/acme/app/issues/10 --jq .state" | ||
|
|
||
| pr "feat(x): add a thing" "" '[{"name":"do-not-merge"}]' | ||
| out=$(run 7) | ||
| assert_exit "the do-not-merge label holds the flip" 3 "$?" | ||
| assert_contains "the label hold names the label" "$out" "do-not-merge" | ||
|
|
||
| pr "feat(x): add a thing" "" '[{"name":"Do-Not-Merge"}]' | ||
| run 7 >/dev/null | ||
| assert_exit "the label matches whatever its case" 3 "$?" | ||
|
|
||
| pr "feat(x): add a thing" "Please do not merge until the release." | ||
| out=$(run 7) | ||
| assert_exit "a 'do not merge' body holds the flip" 3 "$?" | ||
| assert_contains "the body hold says so" "$out" "body" | ||
|
|
||
| pr "feat(x): add a thing" "Please don't merge until the migration lands." | ||
| run 7 >/dev/null | ||
| assert_exit "a \"don't merge\" body holds the flip" 3 "$?" | ||
|
|
||
| pr "feat(x): add a thing" $'Please don’t merge yet.' | ||
| run 7 >/dev/null | ||
| assert_exit "a typographic apostrophe in don't still holds the flip" 3 "$?" | ||
|
|
||
| pr "feat(x): add a thing" $'## Summary\n\nHold: a human merges this\n' | ||
| out=$(run 7) | ||
| assert_exit "a body line starting Hold: holds the merge only, not the flip" 0 "$?" | ||
| assert_silent "a Hold: line reports nothing" "$out" | ||
|
|
||
| pr "feat(x): add a thing" "Adds the do-not-merge label check to the merge lane." | ||
| run 7 >/dev/null | ||
| assert_exit "a body that only names the label is not a hold" 0 "$?" | ||
|
|
||
| pr "test(harness-config): WIP: pin permission suites" "" | ||
| out=$(run 7) | ||
| assert_exit "a WIP marker after the type prefix holds the flip" 3 "$?" | ||
| assert_contains "the WIP hold names the title" "$out" "WIP" | ||
|
|
||
| pr $'WIP: thing \e[2Jnow' "" | ||
| assert_eq "control bytes are stripped from the quoted title" 'title: a WIP marker in "WIP: thing [2Jnow"' "$(run 7)" | ||
|
|
||
| pr "wip(guardrails): wrapper-argv resolver" "" | ||
| run 7 >/dev/null | ||
| assert_exit "a wip type holds the flip" 3 "$?" | ||
|
|
||
| pr "fix(cache): wipe stale entries" "" | ||
| run 7 >/dev/null | ||
| assert_exit "a title word that only starts with wip is not a hold" 0 "$?" | ||
|
|
||
| pr "feat(x): add a thing" "Closes #10, closes #11" "[]" "[$(ref 10),$(ref 11)]" | ||
| echo open >"$TEST_TMPDIR/issue-10" | ||
| echo closed >"$TEST_TMPDIR/issue-11" | ||
| out=$(run 7) | ||
| assert_exit "a closed linked issue refuses the flip" 3 "$?" | ||
| assert_contains "the refusal names the closed issue" "$out" "acme/app#11" | ||
| assert_not_contains "an open linked issue is not reported" "$out" "acme/app#10" | ||
|
|
||
| pr "test(x): WIP: thing" "do not merge" '[{"name":"do-not-merge"}]' "[$(ref 11)]" | ||
| echo closed >"$TEST_TMPDIR/issue-11" | ||
| out=$(run 7) | ||
| assert_eq "every hold is reported, one per line" "4" "$(grep -c . <<<"$out")" | ||
|
kyle-sexton marked this conversation as resolved.
|
||
|
|
||
| assert_contains "a github.com issue names its host" "$(cat "$TEST_TMPDIR/calls")" "--hostname github.com" | ||
|
|
||
| pr "feat(x): add a thing" "" "[]" "[$(ref 10),{\"number\":13,\"url\":null,\"repository\":{\"name\":\"app\",\"owner\":{\"login\":\"acme\"}}}]" | ||
| run 7 >/dev/null 2>&1 | ||
| assert_exit "a malformed closing reference fails closed" 2 "$?" | ||
|
|
||
| rm -f "$TEST_TMPDIR/issue-12" | ||
| pr "feat(x): add a thing" "Closes #12" "[]" "[$(ref 12)]" | ||
| run 7 >/dev/null 2>&1 | ||
| assert_exit "an unreadable linked issue fails closed" 2 "$?" | ||
|
|
||
| rm -f "$TEST_TMPDIR/pr.json" | ||
| run 7 >/dev/null 2>&1 | ||
| assert_exit "an unreadable pull request fails closed" 2 "$?" | ||
|
|
||
| run >/dev/null 2>&1 | ||
| assert_exit "no pull request number exits 1" 1 "$?" | ||
|
|
||
| pr "feat(x): add a thing" "" | ||
| run 7 --repo acme/app >/dev/null | ||
| assert_contains "--repo reaches gh" "$(cat "$TEST_TMPDIR/calls")" "--repo acme/app" | ||
|
|
||
| [[ $FAILED -eq 0 ]] || exit 1 | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.