-
Notifications
You must be signed in to change notification settings - Fork 314
perf(repo-brancher): skip already-synced branches via upfront ls-remote #5033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -124,21 +124,23 @@ func main() { | |||||||||||||||||||||||||||
| failedConfigs.Insert(configInfo) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Multiple CI config variants can share the same org/repo/branch. | ||||||||||||||||||||||||||||
| // Track which ones we've already processed to avoid duplicate | ||||||||||||||||||||||||||||
| // ls-remote calls and git operations. | ||||||||||||||||||||||||||||
| seenBranches := sets.New[string]() | ||||||||||||||||||||||||||||
| if err := o.OperateOnCIOperatorConfigDir(o.ConfigDir, api.WithoutOKD, func(configuration *api.ReleaseBuildConfiguration, repoInfo *config.Info) error { | ||||||||||||||||||||||||||||
| if ignoreSet.Has(repoInfo.Org) || ignoreSet.Has(fmt.Sprintf("%s/%s", repoInfo.Org, repoInfo.Repo)) { | ||||||||||||||||||||||||||||
| logrus.WithField("repo", fmt.Sprintf("%s/%s", repoInfo.Org, repoInfo.Repo)).Info("Skipping due to --ignore") | ||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| logger := config.LoggerForInfo(*repoInfo) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| repoDir := path.Join(gitDir, repoInfo.Org, repoInfo.Repo) | ||||||||||||||||||||||||||||
| if err := os.MkdirAll(repoDir, 0775); err != nil { | ||||||||||||||||||||||||||||
| logger.WithError(err).Fatal("could not ensure git dir existed") | ||||||||||||||||||||||||||||
| branchKey := fmt.Sprintf("%s/%s@%s", repoInfo.Org, repoInfo.Repo, repoInfo.Branch) | ||||||||||||||||||||||||||||
| if seenBranches.Has(branchKey) { | ||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| seenBranches.Insert(branchKey) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| gitCmd := gitCmdFunc(repoDir) | ||||||||||||||||||||||||||||
| logger := config.LoggerForInfo(*repoInfo) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| remote, err := url.Parse(fmt.Sprintf("https://github.com/%s/%s", repoInfo.Org, repoInfo.Repo)) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
|
|
@@ -149,13 +151,22 @@ func main() { | |||||||||||||||||||||||||||
| if o.Confirm { | ||||||||||||||||||||||||||||
| remote.User = url.UserPassword(o.username, token) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| for _, command := range [][]string{{"init"}, {"fetch", "--depth", "1", remote.String(), repoInfo.Branch}} { | ||||||||||||||||||||||||||||
| if err := gitCmd(logger, command...); err != nil { | ||||||||||||||||||||||||||||
| appendFailedConfig(configuration) | ||||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Determine which future branches need work by comparing remote refs | ||||||||||||||||||||||||||||
| // in a single ls-remote call instead of fetching and pushing each one. | ||||||||||||||||||||||||||||
| remoteRefs, err := lsRemoteRefs(logger, remote) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
| appendFailedConfig(configuration) | ||||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| currentSHA := remoteRefs[fmt.Sprintf("refs/heads/%s", repoInfo.Branch)] | ||||||||||||||||||||||||||||
| if currentSHA == "" { | ||||||||||||||||||||||||||||
| logger.Error("Current branch not found on remote.") | ||||||||||||||||||||||||||||
| appendFailedConfig(configuration) | ||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| var branchesNeedingWork []string | ||||||||||||||||||||||||||||
| for _, futureRelease := range o.FutureReleases.Strings() { | ||||||||||||||||||||||||||||
| futureBranch, err := promotion.DetermineReleaseBranch(o.CurrentRelease, futureRelease, repoInfo.Branch) | ||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||
|
|
@@ -167,24 +178,47 @@ func main() { | |||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // when we're initializing the branch, we just want to make sure | ||||||||||||||||||||||||||||
| // it is in sync with the current branch that is promoting | ||||||||||||||||||||||||||||
| logger := logger.WithField("future-branch", futureBranch) | ||||||||||||||||||||||||||||
| command := []string{"ls-remote", remote.String(), fmt.Sprintf("refs/heads/%s", futureBranch)} | ||||||||||||||||||||||||||||
| if err := gitCmd(logger, command...); err != nil { | ||||||||||||||||||||||||||||
| appendFailedConfig(configuration) | ||||||||||||||||||||||||||||
| futureLogger := logger.WithField("future-branch", futureBranch) | ||||||||||||||||||||||||||||
| futureRef := fmt.Sprintf("refs/heads/%s", futureBranch) | ||||||||||||||||||||||||||||
| if remoteRefs[futureRef] == currentSHA { | ||||||||||||||||||||||||||||
| futureLogger.Debug("Already up to date, skipping.") | ||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if !o.Confirm { | ||||||||||||||||||||||||||||
| logger.Info("Would create new branch.") | ||||||||||||||||||||||||||||
| futureLogger.Info("Would create new branch.") | ||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| branchesNeedingWork = append(branchesNeedingWork, futureBranch) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if !o.Confirm || len(branchesNeedingWork) == 0 { | ||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Only init and fetch if there is actual work to do. | ||||||||||||||||||||||||||||
| repoDir := path.Join(gitDir, repoInfo.Org, repoInfo.Repo) | ||||||||||||||||||||||||||||
| if err := os.MkdirAll(repoDir, 0775); err != nil { | ||||||||||||||||||||||||||||
| logger.WithError(err).Fatal("could not ensure git dir existed") | ||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||
|
Comment on lines
+202
to
+204
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n cmd/repo-brancher/main.go | head -250 | tail -100Repository: openshift/ci-tools Length of output: 3695 🏁 Script executed: cat -n cmd/repo-brancher/main.go | head -150Repository: openshift/ci-tools Length of output: 5277 Don't terminate the whole run from one repo's Calling 🛠️ Proposed fix repoDir := path.Join(gitDir, repoInfo.Org, repoInfo.Repo)
if err := os.MkdirAll(repoDir, 0775); err != nil {
- logger.WithError(err).Fatal("could not ensure git dir existed")
- return nil
+ logger.WithError(err).Error("could not ensure git dir existed")
+ appendFailedConfig(configuration)
+ return err
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid observation, but this is pre-existing behavior — the
Comment on lines
+203
to
+204
|
||||||||||||||||||||||||||||
| logger.WithError(err).Fatal("could not ensure git dir existed") | |
| return nil | |
| appendFailedConfig(configuration) | |
| return err |
Copilot
AI
Apr 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New helper lsRemoteRefs introduces non-trivial parsing/retry behavior but has no unit tests. Since this package already has tests for related git helpers (pushBranch, fetchDeeper, fetchUnshallow), it would be good to add tests for lsRemoteRefs parsing (e.g., heads vs HEAD lines, empty output, malformed lines) and error propagation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parsing logic is trivial (strings.Fields on ls-remote output), and the retry/backoff pattern is the same one used in gitCmdFunc which is also untested at that level. Unit testing this would require mocking exec.Command which adds complexity without much value — the interesting behavior (correct skipping of synced branches) is better validated by e2e tests against real repos.
Copilot
AI
Apr 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When ls-remote fails, the retry log and final error drop the command output (b), which makes diagnosing auth/host/key issues difficult. Consider including (possibly truncated) stdout/stderr in the debug log and/or the returned error, similar to how gitCmdFunc wraps errors with output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The remote URL contains credentials, and this function runs outside of gitCmdFunc which handles sanitization. Including raw command output here risks leaking the token into logs. The error from exec.Command already includes the exit code, and the debug-level retry log captures the error — that should be sufficient for diagnosing issues.
Copilot
AI
Apr 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lsRemoteRefs, the retry loop always sleeps after a failed attempt, including after the final (3rd) attempt. This adds unnecessary delay on permanent failures; consider only sleeping when another retry will occur (e.g., guard the sleep with if i < maxAttempts-1).
| time.Sleep(sleepyTime) | |
| sleepyTime *= 2 | |
| if i < 2 { | |
| time.Sleep(sleepyTime) | |
| sleepyTime *= 2 | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repoDiris built withpath.Join, which is intended for slash-separated URL paths and can produce non-native paths on Windows. The rest of this repo typically usesfilepath.Joinfor filesystem paths (e.g.,cmd/private-org-sync/main.go:313), so this should likely usefilepath.Joinand update imports accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct in general, but this tool only runs in Linux CI containers, so
path.Joinworks fine here. The existing code before this PR already usedpath.Joinfor the same purpose, so changing it would be out of scope.