Consolidate backport workflow onto ddev release port-commit - #24470
Consolidate backport workflow onto ddev release port-commit#24470Kyle-Neale wants to merge 13 commits into
Conversation
🎉 All green!🧪 All tests passed 🔄 Datadog auto-retried 1 job - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 13a1035 | Docs | Datadog PR Page | Give us feedback! |
There was a problem hiding this comment.
Pull request overview
This PR consolidates the repository’s automated backporting onto the existing ddev release port-commit implementation, replacing the separate tibdex/backport workflow path. It extends port-commit’s “reset regenerated files to target branch” behavior to cover .deps/ in addition to .in-toto, fixing backports that previously broke when lockfiles were involved.
Changes:
- Extend
port-commitconflict auto-resolution and post-cherry-pick reset behavior to include.deps/alongside.in-toto. - Update/expand
port-committests to cover.deps/conflict handling and non-interactive confirmation skipping (CI/backport workflow path). - Replace the
tibdex/backportGitHub Action with a workflow loop that runsddev --no-interactive release port-commitperbackport/<base>label.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ddev/tests/cli/release/test_port_commit.py |
Updates tests for generated-file preservation, .deps/ conflict resolution, and non-interactive confirmation behavior. |
ddev/src/ddev/cli/release/port_commit.py |
Updates command help text to include .deps/ preservation. |
ddev/src/ddev/cli/release/port_commit_workflow.py |
Generalizes “reset to target” handling to include .deps/ and refactors confirmation prompting for interactive vs CI runs. |
ddev/changelog.d/24470.added |
Documents the new .deps/ reset behavior in release port-commit. |
.github/workflows/backport-pr.yml |
Replaces tibdex/backport with a ddev release port-commit loop keyed off backport/<base> labels. |
.github/chainguard/self.backport.pull-request-target.sts.yaml |
Updates trust-policy documentation to reflect the new workflow and permissions needs. |
Comments suppressed due to low confidence (1)
.github/workflows/backport-pr.yml:24
- The workflow-level
ifcondition usescontains(github.event.label.name, 'backport'), which will also run this job for unrelated labels that merely include the substring (e.g.no-backport). Since the script already expectsbackport/<base>, tighten the condition tostartsWith(..., 'backport/')to avoid unnecessary privileged runs.
if: >
github.event.pull_request.merged
&& (
github.event.action == 'closed'
|| (
github.event.action == 'labeled'
&& contains(github.event.label.name, 'backport')
)
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ef6c2bdea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
AAraKKe
left a comment
There was a problem hiding this comment.
Thanks @Kyle-Neale! Small couple of comments, the biggest one is the god script in the workflow injected into the yaml file.
| GH_TOKEN: ${{ steps.octo-sts.outputs.token }} | ||
| DD_GITHUB_USER: dd-agent-integrations-bot | ||
| run: | | ||
| set -euo pipefail |
There was a problem hiding this comment.
request: lets not have this huge bash script in the middle of the workflow. This can be split in logical steps that are cleaner to handle:
- Are we in a pr with a backport label? If yes, then we should run the backport. We can generate a step with an output that serves a semaphore so the rest of the jobs don't run. If the output is true, then we run the next of the steps
- All the logic getting the name of the labels etc. can be done easily within the ddev port-commit command. We can modify it to include an argument
--from-prand that means we can derive everything we need from the pr itself, use the async client we have with proper testing and test the entire logic. Doing it in a big bash script in a workflow is super error prone and we will find an issue the moment we need to backport something for a release instead of during testing.
My proposal here is to remove this big script, do simple check to get which labels are in the pr and if we have any label Backport/* then should_run output set to true and have another couple of jobs: git setup and backport_pr or something like gated on the should_run output that runs port-commit --from-pr with all the logic built and tested in ddev.
The --from-pr option can then simply add extra steps to the port commit if needed that gets the labels, strip the branch etc. handle all edge cases, test them and make sure they work as expected before shipping.
Replace the tibdex/backport action with a ddev-driven backport that cherry-picks the merged commit per backport/<base> label and resets per-branch regenerated files (.in-toto and now .deps/) to the target branch, so dependency lockfiles don't carry over from the source branch. - Generalize port-commit's .in-toto handling to a path list covering .deps/. - Guard the interactive confirm prompts with app.interactive so the tool runs unattended in CI. - Rework backport-pr.yml to loop the backport/* labels and call ddev --no-interactive release port-commit per base. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Match `.deps/` with startswith so nested paths (e.g. vendor/foo.deps/) are not treated as regenerated; `.in-toto` stays a substring match for its *.in-toto.link files. - Extract `confirm_or_abort` so the interactive confirmation guard lives in one place instead of being duplicated at both prompt sites. - Rename the preserve-step tests to `generated_files` and parametrize the in-toto/deps reset case.
We already have the merge commit SHA from the triggering event; passing it directly avoids resolving PR-<n> back through the GitHub API to the same commit, and drops the dependency on the API's lazily-computed merge_commit_sha.
The backport job's branch push, PR creation, and PR comments all authenticate
with the scoped octo-sts token, so the ambient GITHUB_TOKEN no longer needs
contents/pull-requests write; drop it to read to shrink blast radius under
pull_request_target.
Gate the labeled-event run on startsWith('backport/') instead of
contains('backport') so unrelated labels that merely include the substring
(e.g. no-backport) don't spin up the privileged job.
A backport/<base> label pointing at a branch that does not exist on origin made the per-base git fetch fail under set -e, aborting the whole step so no other backport/* label was processed. Guard the fetch: comment, mark the run failed, and continue to the next base. When port-commit succeeds but the new PR is not yet listed, gh pr list --jq '.[0].url' printed the literal 'null', which is non-empty and produced an 'opened: null' comment. Default the jq expression to an empty string so the comment is skipped instead.
Compress the multi-line inline comments in backport-pr.yml to single lines per the repo's one-line-comment convention. No behavioral change.
Collapse the three near-identical gh pr comment call sites into a single comment_pr helper, centralizing the best-effort (|| true) semantics in one place. No behavioral change.
The open-PR pre-check only detects an OPEN backport PR, so a branch left by a run that failed before opening the PR (or whose PR was later closed) is not auto-cleaned and port-commit's push is rejected non-fast-forward. Document the limitation at the pre-check and, in the failure comment, tell the operator to delete the stale branch and retry. Manual cleanup is the accepted remedy.
3d8949d to
714d920
Compare
Move all backport label parsing, per-base looping, idempotency, and error aggregation into ddev release port-commit --from-pr, backed by unit tests. Slim backport-pr.yml to a should_run gate job plus a gated backport job, and use ddev config override in place of the repo-pointing config set calls.
714d920 to
8cf1f79
Compare
Carry the shared CLI knobs (branch prefix/suffix, labels, no-pr, draft, verify, dry-run) in a frozen PortOptions instead of threading them through build_port_plan, resolve_port_plan, and run_backport_from_pr as a repeated seven-parameter tail. Replace the stringly-typed per-base result tuple with a BackportStatus StrEnum and a BackportResult dataclass, so the overall pass/fail check is an enum comparison rather than a string match. Extract the per-base loop into _port_to_each_base, collapse target_branch plus its explicit flag into a single override_base, and drop run_backport_from_pr's token check in favour of a message override on _resolve_pr. Add a summary-panel test covering the mixed ported/skipped/failed case and register the not-found PR mock explicitly.
…nt typing require_github_user names the get-or-abort contract accurately whether the returned user is used or discarded. Document that the existing-backport-PR check filters on head=owner:branch and therefore relies on the branch being pushed to origin, as PushStep does; a fork-based push flow would silently stop matching. Mirror the real client's Literal['open','closed','all'] on the fake's list_pull_requests state param so test call sites get the same type feedback.
… as planned An explicit --branch-suffix is shared across every base derived from --from-pr, so all bases resolve to the same head branch. After the first opens a PR, the rest are silently skipped as already-ported. Reject the combination unless --target-branch pins a single base. Dry-run ports recorded BackportStatus.PORTED even though nothing was pushed, so the summary implied work happened. Add a PLANNED status used only under --dry-run.
Validation ReportAll 21 validations passed. Show details
|
What does this PR do?
Replaces the
tibdex/backportaction with addev release port-commitloop. For eachbackport/<base>label on a merged PR, the workflow cherry-picks the merged commit onto the release branch and resets per-branch regenerated files —.in-totoand now.deps/— to the target branch. One tested tool now backs both manual and CI backports.Motivation
Auto-backports failed when a merged PR touched
.deps/: the cherry-pick carried the source branch's resolved lockfiles onto the release branch, producing a broken backport. port-commit already reset.in-totothis way; generalizing that to.deps/fixes the failure and collapses the CI-only backport path onto the same tool used manually.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged