From adac3d309ce15fd303ef447930f45c8f605423da Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Tue, 6 Jan 2026 22:35:29 +0530 Subject: [PATCH 1/4] feat: fix `diff_rulesets` method --- sync-team/src/github/mod.rs | 79 +++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 13 deletions(-) diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index b3bf215bf..0705dd907 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -627,24 +627,67 @@ impl SyncGitHub { .github .repo_rulesets(&expected_repo.org, &expected_repo.name)?; - // Build a map of actual rulesets by name - let mut actual_rulesets_map: HashMap = actual_rulesets - .into_iter() - .map(|r| (r.name.clone(), r)) - .collect(); + // Build a map of actual rulesets by branch pattern (the logical identity) + // This allows us to update rulesets even if their name changes + let mut actual_rulesets_by_pattern: HashMap, api::Ruleset> = HashMap::new(); + let mut actual_rulesets_without_pattern: Vec = Vec::new(); + + for ruleset in actual_rulesets { + let patterns = ruleset + .conditions + .as_ref() + .and_then(|c| c.ref_name.as_ref()) + .map(|r| r.include.clone()); + + match patterns { + Some(p) if !p.is_empty() => { + // If multiple rulesets have the same pattern, keep the first one + // and mark others for deletion (they shouldn't exist) + if let Some(existing) = actual_rulesets_by_pattern.insert(p, ruleset) + && let Some(id) = existing.id + { + ruleset_diffs.push(RulesetDiff { + name: existing.name.clone(), + operation: RulesetDiffOperation::Delete(id), + }); + } + } + _ => { + // Rulesets without patterns should be deleted + actual_rulesets_without_pattern.push(ruleset); + } + } + } // Process each branch protection as a potential ruleset for branch_protection in &expected_repo.branch_protections { let expected_ruleset = construct_ruleset(expected_repo, branch_protection); let ruleset_name = expected_ruleset.name.clone(); - if let Some(actual_ruleset) = actual_rulesets_map.remove(&ruleset_name) { - // Ruleset exists, check if it needs updating - // For simplicity, we compare the entire ruleset - // In production, you might want more sophisticated comparison + // Get the expected patterns - construct_ruleset always creates valid patterns + let expected_patterns = expected_ruleset + .conditions + .as_ref() + .and_then(|c| c.ref_name.as_ref()) + .map(|r| r.include.clone()) + .unwrap_or_default(); + + if expected_patterns.is_empty() { + // This shouldn't happen with construct_ruleset, but handle defensively + ruleset_diffs.push(RulesetDiff { + name: ruleset_name, + operation: RulesetDiffOperation::Create(expected_ruleset), + }); + continue; + } + + if let Some(actual_ruleset) = actual_rulesets_by_pattern.remove(&expected_patterns) { + // Ruleset exists for this branch pattern, check if it needs updating + // Compare rules, conditions, enforcement, and name if actual_ruleset.rules != expected_ruleset.rules || actual_ruleset.conditions != expected_ruleset.conditions || actual_ruleset.enforcement != expected_ruleset.enforcement + || actual_ruleset.name != expected_ruleset.name { let id = actual_ruleset.id.unwrap_or(0); ruleset_diffs.push(RulesetDiff { @@ -657,7 +700,7 @@ impl SyncGitHub { }); } } else { - // Ruleset doesn't exist, create it + // Ruleset doesn't exist for this branch pattern, create it ruleset_diffs.push(RulesetDiff { name: ruleset_name, operation: RulesetDiffOperation::Create(expected_ruleset), @@ -665,11 +708,21 @@ impl SyncGitHub { } } - // Any remaining rulesets in actual_rulesets_map should be deleted - for (name, ruleset) in actual_rulesets_map { + // Delete rulesets that have patterns not matching any expected branch protection + for (_, ruleset) in actual_rulesets_by_pattern { if let Some(id) = ruleset.id { ruleset_diffs.push(RulesetDiff { - name, + name: ruleset.name.clone(), + operation: RulesetDiffOperation::Delete(id), + }); + } + } + + // Delete rulesets without valid patterns (shouldn't exist in a managed repo) + for ruleset in actual_rulesets_without_pattern { + if let Some(id) = ruleset.id { + ruleset_diffs.push(RulesetDiff { + name: ruleset.name.clone(), operation: RulesetDiffOperation::Delete(id), }); } From f5be8ec49dfc7b587a1bf191be13da3ce7999d03 Mon Sep 17 00:00:00 2001 From: MarcoIeni <11428655+MarcoIeni@users.noreply.github.com> Date: Tue, 27 Jan 2026 16:59:56 +0100 Subject: [PATCH 2/4] extract patterns function --- sync-team/src/github/api/mod.rs | 9 +++++++++ sync-team/src/github/mod.rs | 13 ++----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/sync-team/src/github/api/mod.rs b/sync-team/src/github/api/mod.rs index debac26ca..cac6452a2 100644 --- a/sync-team/src/github/api/mod.rs +++ b/sync-team/src/github/api/mod.rs @@ -497,6 +497,15 @@ pub(crate) struct Ruleset { pub(crate) rules: Vec, } +impl Ruleset { + pub fn patterns(&self) -> Option> { + self.conditions + .as_ref() + .and_then(|c| c.ref_name.as_ref()) + .map(|r| r.include.clone()) + } +} + #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "lowercase")] pub(crate) enum RulesetTarget { diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 0705dd907..75d434ffd 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -633,11 +633,7 @@ impl SyncGitHub { let mut actual_rulesets_without_pattern: Vec = Vec::new(); for ruleset in actual_rulesets { - let patterns = ruleset - .conditions - .as_ref() - .and_then(|c| c.ref_name.as_ref()) - .map(|r| r.include.clone()); + let patterns = ruleset.patterns(); match patterns { Some(p) if !p.is_empty() => { @@ -665,12 +661,7 @@ impl SyncGitHub { let ruleset_name = expected_ruleset.name.clone(); // Get the expected patterns - construct_ruleset always creates valid patterns - let expected_patterns = expected_ruleset - .conditions - .as_ref() - .and_then(|c| c.ref_name.as_ref()) - .map(|r| r.include.clone()) - .unwrap_or_default(); + let expected_patterns = expected_ruleset.patterns().unwrap_or_default(); if expected_patterns.is_empty() { // This shouldn't happen with construct_ruleset, but handle defensively From ca0a0f09152f4f6aa5c4b0a2bb619ce4cefea1dc Mon Sep 17 00:00:00 2001 From: MarcoIeni <11428655+MarcoIeni@users.noreply.github.com> Date: Tue, 27 Jan 2026 18:22:37 +0100 Subject: [PATCH 3/4] fix --- sync-team/src/github/mod.rs | 47 +++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 75d434ffd..3e054cfea 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -638,14 +638,17 @@ impl SyncGitHub { match patterns { Some(p) if !p.is_empty() => { // If multiple rulesets have the same pattern, keep the first one - // and mark others for deletion (they shouldn't exist) - if let Some(existing) = actual_rulesets_by_pattern.insert(p, ruleset) - && let Some(id) = existing.id - { - ruleset_diffs.push(RulesetDiff { - name: existing.name.clone(), - operation: RulesetDiffOperation::Delete(id), - }); + // and mark later ones for deletion (they shouldn't exist). + if actual_rulesets_by_pattern.contains_key(&p) { + // It's a duplicate, so delete it. + if let Some(id) = ruleset.id { + ruleset_diffs.push(RulesetDiff { + name: ruleset.name.clone(), + operation: RulesetDiffOperation::Delete(id), + }); + } + } else { + actual_rulesets_by_pattern.insert(p, ruleset); } } _ => { @@ -680,15 +683,25 @@ impl SyncGitHub { || actual_ruleset.enforcement != expected_ruleset.enforcement || actual_ruleset.name != expected_ruleset.name { - let id = actual_ruleset.id.unwrap_or(0); - ruleset_diffs.push(RulesetDiff { - name: ruleset_name, - operation: RulesetDiffOperation::Update( - id, - actual_ruleset, - expected_ruleset, - ), - }); + match actual_ruleset.id { + Some(id) => { + ruleset_diffs.push(RulesetDiff { + name: ruleset_name, + operation: RulesetDiffOperation::Update( + id, + actual_ruleset, + expected_ruleset, + ), + }); + } + None => { + // A ruleset without an id cannot be updated; fall back to creating. + ruleset_diffs.push(RulesetDiff { + name: ruleset_name, + operation: RulesetDiffOperation::Create(expected_ruleset), + }); + } + } } } else { // Ruleset doesn't exist for this branch pattern, create it From b5540361ef46880bbd35a155fac3186dd0a713ea Mon Sep 17 00:00:00 2001 From: MarcoIeni <11428655+MarcoIeni@users.noreply.github.com> Date: Tue, 27 Jan 2026 18:27:45 +0100 Subject: [PATCH 4/4] clippy --- sync-team/src/github/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 3e054cfea..ed81b3749 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -7,7 +7,7 @@ use crate::Config; use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings}; use log::debug; use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot}; -use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, hash_map}; use std::fmt::Write; pub(crate) use self::api::{GitHubApiRead, GitHubWrite, HttpClient}; @@ -639,7 +639,9 @@ impl SyncGitHub { Some(p) if !p.is_empty() => { // If multiple rulesets have the same pattern, keep the first one // and mark later ones for deletion (they shouldn't exist). - if actual_rulesets_by_pattern.contains_key(&p) { + if let hash_map::Entry::Vacant(e) = actual_rulesets_by_pattern.entry(p) { + e.insert(ruleset); + } else { // It's a duplicate, so delete it. if let Some(id) = ruleset.id { ruleset_diffs.push(RulesetDiff { @@ -647,8 +649,6 @@ impl SyncGitHub { operation: RulesetDiffOperation::Delete(id), }); } - } else { - actual_rulesets_by_pattern.insert(p, ruleset); } } _ => {