Repository navigation
feat(backport): wire backport-fix-imports behind fix-imports with a sticky pr comment - #332
vcauesantos wants to merge 3 commits into
Conversation
loft-bot
left a comment
There was a problem hiding this comment.
Panel review: 1 blocking, 4 quality notes, 9 lanes.
Blocking concerns
.github/actions/backport-fix-imports/action.yml:58: apush: 'false'dry run still posts a real sticky comment naming a commit that was never pushed.
What was checked: correctness, security, test-quality, operability, reuse, arch, typo, infra, pr-metadata, plus a check for sibling PRs that delete symbols this diff calls (#330 deletes none). Skipped: dead-code, e2e family, gap-analyzer/verifier, entry-point-fidelity, layout (nothing deleted, no e2e files, no customer-bug ticket, not a restructure).
Quality notes (non-blocking)
.github/actions/backport-fix-imports/src/report.py:56: consider: Could we fetch the/userlogin before writing any output? If that call fails afterbodyandpr-numberare written, the comment step runs with an empty author. sticky-pr-comment then exits, andcontinue-on-errorhides it..github/actions/backport-fix-imports/src/fix.py:392: consider: Could a test assert the per-file reason for a blocked move innotes["left"]? Only the conflict-marker reason is checked, and this text goes straight into the PR comment. This becomes blocking if a wrong or missing reason is the only thing telling the assignee why a file was left..github/actions/backport-fix-imports/src/fix.py:109: consider: Couldgithub()andbackport_pr()move into report.py, or into a small shared module that #330 can reuse? fix.py never calls them, and report.py imports the whole rewrite module just to reach them..github/actions/backport-fix-imports/action.yml: consider: Could theconclusionoutput description, and the generated README table, list "no backport branch" as a cause ofskipped? This PR adds that path, but the contract still names only the old three causes.
…ore writing outputs
|
Notes, fixed in b157df0: the login is read before any output is written, a test checks the blocked-move reason, and |
Summary
Stacked on #329. This is the script-only rollout from Piotr's feedback:
backport.yamlcan runbackport-fix-importson every backport branch. There's no AI code here; the AI pass comes later in #330.backport.yamlgets afix-importsflag, off by default. When it's on, a job runs the action on eachbackport/<target>/pr-<n>branch after the cherry-picks, using the same bot token as the cherry-pick.backport-to-*labels. It runs first, so the list exists even if a cherry-pick fails.skippedinstead offailed.gofmtruns without the push token in its environment.For
vclusterandvcluster-pro, which callbackport.yaml@backport/v1, nothing changes until they setfix-imports: true. They only get the extra one-line step that lists the targets.Flow
flowchart TD A[PR merged with backport-to labels] --> B[Bot cherry-picks and opens backport PRs] B --> C{fix-imports on?} C -->|No| Done[Nothing else runs] C -->|Yes| D[One job per backport branch] D --> E{Backport branch exists?} E -->|No| S[Skip] E -->|Yes| F[Script fixes imports and pushes one commit] F --> G[Sticky comment: what was fixed and what is left]If any step fails, nothing is pushed and the backport PR stays as the bot opened it.
Test plan
make test-backport-fix-imports: 73 pytest tests, including the comment body and its outputs, the skip when there's no backport branch, and the notes the script writes.make lint(actionlint, zizmor) andmake check-docsare clean.loft-enterprisereplay branches (see feat(backport): run backport-fix-imports with an ai pass gated on go vet #330, section Real run on loft-enterprise). The script commits for #8643 and #8719 are identical to the human fixes.backport.yaml(target list, matrix, branch name) and the sticky comment. The first real backport afterloft-enterpriseturns the flag on will be their test.Rollout
backport-fix-imports/v1tag and advancebackport/v1.loft-enterprisepinsbackport.yamlto a commit from around July. Move that pin first, withfix-importsstill off.fix-imports: trueinloft-enterpriseand watch real backports.Related to DEVOPS-1648
Related to DEVOPS-1671