Skip to content

Commit fb24726

Browse files
ci: label user-prefixed backport branches too (#15105)
## The workflow does not catch the branches it exists for `pr-label-backport.yaml`'s header states its purpose plainly — *"Guarantees that every backport PR carries the `backport` label, **whoever opened it**"* — and explains that manual backports are the case that matters, because `pr-backport.yaml` only labels the PRs it creates itself, and hands off to a human when the cherry-pick conflicts. But `isBackportBranchForBase` matched only the exact branch the workflow generates: ```ts const BACKPORT_BRANCH_PATTERN = /^backport-\d+-to-(.+)$/ ``` A human finishing a conflicted cherry-pick does not produce that. They produce `<user>/backport-<pr>-to-<target>` — what `gh` and most local branch conventions give you. ## This week's worked example **#15102** and **#15103** (`jaeone94/backport-14984-to-core-1.49` and `…-cloud-1.49`) were opened by hand after `pr-backport.yaml` failed on #14984's cherry-pick, and carry the `backport` label **only because someone applied it manually**. That matters because an unlabelled backport is invisible to `backport-auto-merge.yaml` and `pr-assign-release-sheriff.yaml` — both sweep `gh pr list --label backport` — so it sits open until somebody notices. Which is exactly what #14018#14021 are cited for in the workflow's own header. Surveying every backport-titled PR in the repo: the bot's `backport-<n>-to-<target>` branches are all labelled, and the only user-prefixed pair is the one that needed a human. ## Only the prefix widens ```diff -const BACKPORT_BRANCH_PATTERN = /^backport-\d+-to-(.+)$/ +const BACKPORT_BRANCH_PATTERN = /^(?:.*\/)?backport-\d+-to-(.+)$/ ``` The `\d+-to-` core and the existing base check still decide what counts as a backport, so this does **not** loosen the definition: | branch | base | before | after | | --- | --- | --- | --- | | `backport-14979-to-core-1.49` | `core/1.49` | ✅ | ✅ | | `jaeone94/backport-14984-to-core-1.49` | `core/1.49` | ❌ | ✅ | | `jaeone94/backport-14984-to-core-1.49` | `cloud/1.49` | ❌ | ❌ *(target ≠ base)* | | `glary/backport-resilient-partial-failure` | `core/1.49` | ❌ | ❌ | | `deepme987/infra/backport-assign-author` | `main` | ❌ | ❌ | The last two are real branches in this repo — *about* backporting rather than backports. A prefix must not turn them into one, and it doesn't: they have no `<digits>-to-` segment. ## Verification `backport-label.test.ts` **16/16**, with three new cases covering the prefixed form, the prefixed-but-wrong-base rejection, and the two real non-backport branches above. Mutation-checked rather than asserted green — reverting the regex to `^backport-` fails exactly one test (the new prefixed case) and leaves the other 15 passing. ## Disclosure I hit this myself from the other direction. My manual backports #15095/#15096 used `backport/<pr>-to-<target>` — a **slash** where the convention uses a hyphen. That one is genuinely off-convention rather than a gap in the rule, so this PR deliberately does **not** widen the pattern to bless it; I fixed those branches' labels by hand instead. Co-authored-by: t <t@t.t>
1 parent 82bad2e commit fb24726

2 files changed

Lines changed: 54 additions & 1 deletion

File tree

scripts/cicd/backport-label.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,46 @@ describe('isBackportBranchForBase', () => {
9393
isBackportBranchForBase('backport-abc-to-cloud-1.47', 'cloud/1.47')
9494
).toBe(false)
9595
})
96+
97+
it('recognises the user-prefixed form a human finishing a cherry-pick creates', () => {
98+
// #15102 and #15103 are the worked example: opened by hand after the
99+
// workflow's cherry-pick conflicted, and labelled by hand because this
100+
// returned false for them.
101+
expect(
102+
isBackportBranchForBase(
103+
'jaeone94/backport-14984-to-core-1.49',
104+
'core/1.49'
105+
)
106+
).toBe(true)
107+
expect(
108+
isBackportBranchForBase(
109+
'deepme987/infra/backport-123-to-cloud-1.49',
110+
'cloud/1.49'
111+
)
112+
).toBe(true)
113+
})
114+
115+
it('still checks the encoded target against the base when prefixed', () => {
116+
expect(
117+
isBackportBranchForBase(
118+
'jaeone94/backport-14984-to-core-1.49',
119+
'cloud/1.49'
120+
)
121+
).toBe(false)
122+
})
123+
124+
it('rejects a prefixed branch that only mentions backport', () => {
125+
// Real branches in this repo. A prefix must not turn them into backports.
126+
expect(
127+
isBackportBranchForBase(
128+
'glary/backport-resilient-partial-failure',
129+
'core/1.49'
130+
)
131+
).toBe(false)
132+
expect(
133+
isBackportBranchForBase('deepme987/infra/backport-assign-author', 'main')
134+
).toBe(false)
135+
})
96136
})
97137

98138
describe('planBackportLabels', () => {

scripts/cicd/backport-label.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,20 @@ export function toSafeBranchName(branch: string): string {
3131
return branch.replaceAll('/', '-')
3232
}
3333

34-
const BACKPORT_BRANCH_PATTERN = /^backport-\d+-to-(.+)$/
34+
/**
35+
* The optional leading path segment is what makes this useful for the manual
36+
* backports this workflow exists to catch. `pr-backport.yaml` creates
37+
* `backport-<pr>-to-<target>`, but a human finishing a conflicted cherry-pick
38+
* by hand almost always ends up with `<user>/backport-<pr>-to-<target>` —
39+
* that is what `gh` and most local conventions produce. Two such PRs appeared
40+
* in one week (#15102, #15103) and had to be labelled by hand.
41+
*
42+
* Widening the prefix does not widen what counts as a backport: the
43+
* `\d+-to-` core and the base check below still do that work, so branches
44+
* like `glary/backport-resilient-partial-failure` — real branches in this
45+
* repo, about backporting rather than backports — stay rejected.
46+
*/
47+
const BACKPORT_BRANCH_PATTERN = /^(?:.*\/)?backport-\d+-to-(.+)$/
3548

3649
/**
3750
* True when `headRef` is the backport branch that `pr-backport.yaml` would

0 commit comments

Comments
 (0)