Conversation
Add `scripts/plugins/sync-branch.ts` and the Prow periodic job `periodic-sync-branch-tidb-feature-release-8.5-fts` to keep `pingcap/tidb` `feature/release-8.5-fts` up to date with `release-8.5`. The sync uses the GitHub Merge API to create a regular merge commit, so the feature branch commits and its `v8.5.*-fts` git tags are preserved. On merge conflicts it opens a conflict resolution pull request and fails the job. Document the mechanism in `docs/guides/feature-branch-sync.md`.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR introduces a script to automate syncing feature branches with their base release branch using the GitHub Merge API. A Prow periodic job schedules the syncing operation daily. The implementation avoids destructive operations like rebases or force-pushes, preserving both feature-specific commits and git tags. The code is well-structured overall, with detailed documentation, unit tests, and adherence to established practices. However, there are some opportunities for improvement in error handling, efficiency, and maintainability.
Critical Issues
-
Error handling for webhook notifications (lines 263–273 of
sync-branch.ts):- If the webhook notification fails, the error is only logged, and the script continues execution. This could lead to undetected failures in alerting critical conflicts.
- Suggested Solution:
if (!response.ok) { throw new Error(`Failed to send notification: HTTP ${response.status}`); }
-
Missing retry mechanism for GitHub API calls (multiple locations in
sync-branch.ts):- Network or transient errors may cause API calls (e.g.,
mergeSourceIntoTarget,branchExists) to fail prematurely. Without retries, the job might report a failure unnecessarily. - Suggested Solution: Wrap API calls in a retry mechanism. For example:
async function retry<T>(fn: () => Promise<T>, retries = 3): Promise<T> { for (let attempt = 1; attempt <= retries; attempt++) { try { return await fn(); } catch (error) { if (attempt === retries) throw error; await new Promise((resolve) => setTimeout(resolve, 1000)); } } }
- Network or transient errors may cause API calls (e.g.,
Code Improvements
-
Avoid duplicate conflict pull requests (lines 174–186 of
sync-branch.ts):- The
ensureConflictPullRequestfunction only checks for PRs whereheadmatches the source branch andbasematches the target branch. This could lead to duplicate PRs when multiple syncs are attempted with unresolved conflicts. - Suggested Solution:
Improve filtering logic to include PRs where the title matchesbuildConflictPullRequestTitle(sourceBranch, targetBranch).
- The
-
Optimize notification construction (lines 252–260 of
sync-branch.ts):- The notification card is constructed inline. If reused across multiple places, centralizing this logic would improve readability and code reuse.
- Suggested Solution:
Factor outbuildNotificationCard(title, message)into a helper function.
-
Reduce verbosity in dry-run mode (lines 126–128 of
sync-branch.ts):- Logging each target branch in dry-run mode can clutter the console for large sets of branches.
- Suggested Solution:
Aggregate results into a summary log after processing all branches.
Best Practices
-
Unit test coverage for edge cases (
sync-branch.test.ts):- The tests do not cover scenarios where:
- API calls fail (e.g., 500 server errors).
- The
normalizeTargetBranchesfunction is given unusual input (e.g., duplicate empty branches).
- Suggested Solution: Add tests for these edge cases:
Deno.test("normalizeTargetBranches handles empty inputs", () => { assertEquals(normalizeTargetBranches(["", ""]), []); });
- The tests do not cover scenarios where:
-
Missing integration tests:
- While unit tests exist, there are no integration tests to validate behavior with mock GitHub API responses.
- Suggested Solution: Use a mocking library to simulate GitHub API responses for end-to-end tests.
-
Environment variable validation (lines 309–310 in
sync-branch.ts):- Critical inputs like
github_private_tokenare not validated for presence or format. Missing tokens could lead to runtime errors. - Suggested Solution:
if (!github_private_token) { console.error("Missing GitHub API token."); Deno.exit(1); }
- Critical inputs like
Conclusion
The PR is well-documented, functional, and adheres to best practices in many areas, but error handling and test coverage could be improved. Addressing the critical issues and suggested improvements would enhance resilience and maintainability.
|
Keeping this pull request as a draft on purpose. Please do not enable or merge it yet. We will mark it ready for review and merge it after the initial When we are ready to enable it:
|
Drive the branch sync from `scripts/plugins/sync-branches.yaml` so the scheduled job keeps `feature/release-8.5-fts` up to date with its base release branch for the whole v8.5 product line: - pingcap/tidb, pingcap/tiflash, pingcap/ticdc, pingcap/kvproto, pingcap/tipb, PingCAP-QE/tidb-test, tikv/tikv, tikv/pd, tikv/client-c from `release-8.5` - tikv/client-go from `tidb-8.5` Rename the periodic job to `periodic-sync-feature-branches-with-release-8.5` and pass `--config`. The single-repository CLI arguments are kept for ad-hoc runs.
Move `scripts/plugins/sync-branches.yaml` to `configs/sync-branches.yaml` and update the Prow job and the guide to the new path.
What problem does this PR solve?
Keep the long-lived
feature/release-8.5-ftsbranch of the whole v8.5 product line automatically up to date with its base release branch, while preserving each feature branch's own commits and itsv8.5.*-ftsgit tags.What changed and how does it work?
scripts/plugins/sync-branch.ts, which merges a source branch into one or more target branches through the GitHub Merge API:201 Createdcreates a regular merge commit on the target branch;204 No Contentmeans the target already contains the source;409 Conflictopens (or reuses) a conflict resolution pull request and fails the job so it is visible.configs/sync-branches.yaml, the repository mapping consumed by the job. The same script still accepts--owner/--repository/--source_branch/--target_branchfor ad-hoc single-repository runs.periodic-sync-feature-branches-with-release-8.5(daily at 02:00 UTC) inprow-jobs/pingcap-qe/ci/periodics.yaml.scripts/plugins/sync-branch.test.tsunit tests for the pure helpers.docs/guides/feature-branch-sync.mdand link it fromdocs/guides/README.md.Synced repositories (
feature/release-8.5-fts):pingcap/tidbrelease-8.5pingcap/tiflashrelease-8.5pingcap/ticdcrelease-8.5pingcap/kvprotorelease-8.5pingcap/tipbrelease-8.5PingCAP-QE/tidb-testrelease-8.5tikv/tikvrelease-8.5tikv/pdrelease-8.5tikv/client-crelease-8.5tikv/client-gotidb-8.5Because the sync uses a merge commit and never resets, rebases or force-pushes the target branch, the feature-specific commits stay reachable and existing git tags are not orphaned. The job never calls any tag API;
v8.5.*-ftstags are still created by the normal feature-branch build/release flow (scripts/flow/build/versioning-strategy.ts).Intended order of operations:
v8.5.xis released on the base release branch -> the next scheduled sync merges it intofeature/release-8.5-fts-> the feature branch release flow produces and tagsv8.5.x-fts.Check List
deno test --allow-net --allow-read scripts/plugins/sync-branch.test.tsdeno check,deno lint,deno fmt --checkyqparse of the modified Prow job YAML and the new config YAML.ci/update-prow-job-kustomization.sh(no kustomization change needed)Test result