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), }); }