Skip to content

feat(backport): wire backport-fix-imports behind fix-imports with a sticky pr comment - #332

Open
vcauesantos wants to merge 3 commits into
devops-1648/backport-fix-importsfrom
devops-1648/backport-fix-imports-wiring
Open

vcauesantos wants to merge 3 commits into
devops-1648/backport-fix-importsfrom
devops-1648/backport-fix-imports-wiring

Conversation

@vcauesantos

@vcauesantos vcauesantos commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Stacked on #329. This is the script-only rollout from Piotr's feedback: backport.yaml can run backport-fix-imports on every backport branch. There's no AI code here; the AI pass comes later in #330.

  • backport.yaml gets a fix-imports flag, off by default. When it's on, a job runs the action on each backport/<target>/pr-<n> branch after the cherry-picks, using the same bot token as the cherry-pick.
  • A one-line step at the start of the backport job lists the targets from the backport-to-* labels. It runs first, so the list exists even if a cherry-pick fails.
  • One sticky comment on the backport PR tells the assignee what the script fixed and which files are left, with the reason for each. There's no comment when nothing changed and nothing is left.
  • A label with no backport branch, such as a legacy line, is now skipped instead of failed.
  • gofmt runs without the push token in its environment.

For vcluster and vcluster-pro, which call backport.yaml@backport/v1, nothing changes until they set fix-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]
Loading

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) and make check-docs are clean.
  • The action already ran for real on loft-enterprise replay 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.
  • Not run for real yet: the new job in backport.yaml (target list, matrix, branch name) and the sticky comment. The first real backport after loft-enterprise turns the flag on will be their test.

Rollout

  1. Merge feat(backport-fix-imports): rewrite module paths on backports to older module majors #329, then this PR. Create the backport-fix-imports/v1 tag and advance backport/v1.
  2. loft-enterprise pins backport.yaml to a commit from around July. Move that pin first, with fix-imports still off.
  3. Then set fix-imports: true in loft-enterprise and watch real backports.

Related to DEVOPS-1648
Related to DEVOPS-1671

@loft-bot loft-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panel review: 1 blocking, 4 quality notes, 9 lanes.

Blocking concerns

  • .github/actions/backport-fix-imports/action.yml:58: a push: '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 /user login before writing any output? If that call fails after body and pr-number are written, the comment step runs with an empty author. sticky-pr-comment then exits, and continue-on-error hides it.
  • .github/actions/backport-fix-imports/src/fix.py:392: consider: Could a test assert the per-file reason for a blocked move in notes["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: Could github() and backport_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 the conclusion output description, and the generated README table, list "no backport branch" as a cause of skipped? This PR adds that path, but the contract still names only the old three causes.

Comment thread .github/actions/backport-fix-imports/action.yml Outdated
@vcauesantos

Copy link
Copy Markdown
Contributor Author

Notes, fixed in b157df0: the login is read before any output is written, a test checks the blocked-move reason, and conclusion now lists "no backport branch". I left github() and backport_pr() in fix.py: it is already the shared module that report.py and later ai.py import for emit, git and the notes, so a separate file would only spread the code around.

@vcauesantos
vcauesantos marked this pull request as ready for review October 23, 2026 22:35
@vcauesantos
vcauesantos requested a review from a team as a code owner October 23, 2026 22:35
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.

2 participants