fix(private-org-sync): batch ls-remote calls per repo instead of per branch#5027
fix(private-org-sync): batch ls-remote calls per repo instead of per branch#5027petr-muller wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughIntroduces a two-stage git sync flow: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: petr-muller 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 |
…branch Previously, mirror() called ls-remote for both source and destination on every branch, resulting in 2*N network calls per repo where N is the number of branches. Since ls-remote --heads returns ALL branches at once, we can call it once per repo and pass the results to mirror(). This moves git init, remote setup, and ls-remote calls from mirror() into the main loop where repos are already grouped, reducing network calls from 2*N to 2 per repo. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
petr-muller
force-pushed
the
git-sync-speedup-4
branch
from
48f29ee to
136df2b
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/hold Still need to review the code properly and address CodeRabbit reviews |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Pull request overview
Reduces git ls-remote network calls in private-org-sync by batching branch head discovery once per repository (instead of per-branch), and passing those results into per-branch mirroring.
Changes:
- Update
gitSyncer.mirror()to accept pre-fetched source/destination branch heads and a pre-built destination URL. - Move destination/source
ls-remote(and related URL construction) into the per-repo loop inmain(). - Refactor
TestMirrorto injectsrcHeads/dstHeadsand a destination URL rather than expectingls-remotecalls insidemirror().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/private-org-sync/main.go | Batches per-repo ls-remote calls and updates mirror() to consume pre-fetched heads/URL. |
| cmd/private-org-sync/main_test.go | Adjusts TestMirror expectations to reflect the refactor (no ls-remote inside mirror()). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/private-org-sync/main_test.go (1)
258-258: Minor: Error ignored in test setup.The error from
url.Parseis discarded. While this is fine for a hardcoded valid URL in tests, explicitly handling it would be more defensive.♻️ Optional improvement
- destUrl, _ := url.Parse(fmt.Sprintf("https://%s@github.com/%s/%s", token, destOrg, repo)) + destUrl, err := url.Parse(fmt.Sprintf("https://%s@github.com/%s/%s", token, destOrg, repo)) + if err != nil { + t.Fatalf("failed to parse destUrl: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/private-org-sync/main_test.go` at line 258, The test currently ignores the error returned by url.Parse when building destUrl; update the test to capture the returned error (e.g., destUrl, err := url.Parse(...)) and assert or fail on it (using t.Fatalf/require.NoError) so any unexpected parse failure for the constructed URL using token, destOrg, and repo is handled instead of silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/private-org-sync/main_test.go`:
- Line 258: The test currently ignores the error returned by url.Parse when
building destUrl; update the test to capture the returned error (e.g., destUrl,
err := url.Parse(...)) and assert or fail on it (using t.Fatalf/require.NoError)
so any unexpected parse failure for the constructed URL using token, destOrg,
and repo is handled instead of silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6ae7923-e687-4285-9e9d-d93e163d9f02
📒 Files selected for processing (2)
cmd/private-org-sync/main.gocmd/private-org-sync/main_test.go
|
/test images |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
Extract the per-repo sync logic from main() into a syncRepo method on gitSyncer. This makes the repo initialization, ls-remote batching, and branch mirroring flow independently testable. Add TestSyncRepo with 6 test cases covering: branches in sync, one branch needing sync, dst/src ls-remote failures (with and without failOnNonexistentDst), and init failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/test ci/prow/codegen ci/prow/format |
|
/test codegen format |
|
/test images |
|
@CodeRabbit help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
@petr-muller: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
1 similar comment
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
Previously,
mirror()calledls-remotefor both source and destination on every branch, resulting in 2×N network calls per repo where N is the number of branches. Sincels-remote --headsreturns ALL branches at once, we can call it once per repo and pass the results tomirror().This moves git init, remote setup, and ls-remote calls from
mirror()into the main loop where repos are already grouped, reducing network calls from 2×N to 2 per repo.Stacked on #5023 (merged).