Skip to content

branchprotector: remove protection from excluded branches #478

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+41 −0
Open

branchprotector: remove protection from excluded branches #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions cmd/branchprotector/protect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,11 +326,13 @@ func (p *protector) UpdateRepo(orgName string, repoName string, repo config.Repo
}

branches := map[string]github.Branch{}
var allBranches []github.Branch
for _, onlyProtected := range []bool{false, true} { // put true second so b.Protected is set correctly
bs, err := p.client.GetBranches(orgName, repoName, onlyProtected)
if err != nil {
return fmt.Errorf("list branches: %w", err)
}
allBranches = append(allBranches, bs...)
for _, b := range bs {
_, ok := repo.Branches[b.Name]
if !ok && branchInclusions != nil && branchInclusions.MatchString(b.Name) {
Expand All @@ -346,6 +348,27 @@ func (p *protector) UpdateRepo(orgName string, repoName string, repo config.Repo
}
}

// Handle excluded branches that are currently protected and need removal
if branchExclusions != nil {
seen := make(map[string]bool)
Copy link
Member

@Prucek Prucek Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this map needed?

Copy link
Author

@kaovilai kaovilai Jun 17, 2025

Choose a reason for hiding this comment

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

Problem

The allBranches slice can contain duplicate entries for the same branch because it's populated from two separate API calls:

  for _, onlyProtected := range []bool{false, true} { // runs twice
      bs, err := p.client.GetBranches(orgName, repoName, onlyProtected)
      allBranches = append(allBranches, bs...) // can add same branch twice
  }

Without Deduplication

If a branch like konflux-test appears in both API responses, we would send two removal requests:

  1. First iteration: {Org: "org", Repo: "repo", Branch: "konflux-test", Request: nil}
  2. Second iteration: {Org: "org", Repo: "repo", Branch: "konflux-test", Request: nil}

With seen Map

  if b.Protected && branchExclusions.MatchString(b.Name) && !seen[b.Name] {
      seen[b.Name] = true  // Mark as processed
      // Send removal request only once
  }

The seen map ensures each excluded protected branch gets exactly one removal request, regardless of how many times it appears in allBranches.

This prevents:

  • Duplicate API calls to GitHub
  • Confusing log messages
  • Potential race conditions in the removal process

Copy link
Author

@kaovilai kaovilai Jun 17, 2025

Choose a reason for hiding this comment

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

Perhaps if p.client.GetBranches(orgName, repoName, false) return all branches we can initialize allBranches once.

Copy link
Member

@Prucek Prucek Jun 18, 2025

Choose a reason for hiding this comment

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

okay, get it. Thanks!

for _, b := range allBranches {
if b.Protected && branchExclusions.MatchString(b.Name) && !seen[b.Name] {
seen[b.Name] = true
// Check if this branch is not in the branches map (i.e., it was excluded)
if _, inBranches := branches[b.Name]; !inBranches {
logrus.Infof("%s/%s=%s: removing protection from excluded branch", orgName, repoName, b.Name)
p.updates <- requirements{
Org: orgName,
Repo: repoName,
Branch: b.Name,
Request: nil, // nil Request triggers RemoveBranchProtection
// Flow: updates channel -> configureBranches() -> client.RemoveBranchProtection()
}
}
}
}
}

var apps, collaborators, teams []string
if p.verifyRestrictions {
apps, err = p.authorizedApps(orgName)
Expand Down
18 changes: 18 additions & 0 deletions cmd/branchprotector/protect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1109,6 +1109,12 @@ branch-protection:
`,

expected: []requirements{
{
Org: "kubernetes",
Repo: "test-infra",
Branch: "skip",
Request: nil, // nil Request triggers RemoveBranchProtection
},
{
Org: "kubernetes",
Repo: "test-infra",
Expand All @@ -1133,6 +1139,18 @@ branch-protection:
- sk.*
`,
expected: []requirements{
{
Org: "kubernetes",
Repo: "test-infra",
Branch: "skip",
Request: nil, // nil Request triggers RemoveBranchProtection
},
{
Org: "kubernetes",
Repo: "test-infra",
Branch: "foobar1",
Request: nil, // nil Request triggers RemoveBranchProtection
},
{
Org: "kubernetes",
Repo: "test-infra",
Expand Down