From 8e7796f9373d9b89b617d35f0bde9bec4769d2a7 Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Mon, 22 Dec 2025 23:16:00 +0530 Subject: [PATCH 1/5] feat: implement bypass actor ruleset --- sync-team/src/github/api/mod.rs | 277 +++++++++++++++++++++++++++++- sync-team/src/github/api/write.rs | 77 +++++++++ sync-team/src/github/mod.rs | 193 +++++++++++++++++++-- 3 files changed, 531 insertions(+), 16 deletions(-) diff --git a/sync-team/src/github/api/mod.rs b/sync-team/src/github/api/mod.rs index debac26ca..7005b6960 100644 --- a/sync-team/src/github/api/mod.rs +++ b/sync-team/src/github/api/mod.rs @@ -521,25 +521,110 @@ pub(crate) enum RulesetEnforcement { #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub(crate) struct RulesetBypassActor { - pub(crate) actor_id: i64, + /// The ID of the actor that can bypass a ruleset. + /// - Required for Integration, RepositoryRole, and Team actor types (use Some(ActorId::Id(n))) + /// - Must be 1 for OrganizationAdmin (use Some(ActorId::Id(1))) + /// - Must be null for DeployKey (use Some(ActorId::Null)) + /// - Omitted for EnterpriseOwner (use None) + #[serde( + skip_serializing_if = "Option::is_none", + default, + deserialize_with = "deserialize_actor_id" + )] + pub(crate) actor_id: Option, pub(crate) actor_type: RulesetActorType, - pub(crate) bypass_mode: RulesetBypassMode, + /// The bypass mode for the actor. Defaults to "always" per GitHub API. + /// - always: Actor can always bypass + /// - pull_request: Actor can only bypass on pull requests (not applicable for DeployKey) + /// - exempt: Rules won't run for actor, no audit entry created + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) bypass_mode: Option, } +/// Custom deserializer for actor_id that distinguishes between null and missing +fn deserialize_actor_id<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + use serde::{Deserialize, de}; + + struct ActorIdVisitor; + + impl<'de> de::Visitor<'de> for ActorIdVisitor { + type Value = Option; + + fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + formatter.write_str("an integer, null, or missing field") + } + + fn visit_none(self) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Null)) + } + + fn visit_some(self, deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let id = i64::deserialize(deserializer)?; + Ok(Some(ActorId::Id(id))) + } + + fn visit_unit(self) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Null)) + } + + fn visit_i64(self, v: i64) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Id(v))) + } + + fn visit_u64(self, v: u64) -> Result + where + E: de::Error, + { + Ok(Some(ActorId::Id(v as i64))) + } + } + + deserializer.deserialize_option(ActorIdVisitor) +} + +/// Represents the actor_id field which can be a number or null #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -#[serde(rename_all = "snake_case")] +#[serde(untagged)] +pub(crate) enum ActorId { + /// A specific actor ID (used for Integration, RepositoryRole, Team, OrganizationAdmin) + Id(i64), + /// Explicitly null (required for DeployKey) + Null, +} + +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "PascalCase")] pub(crate) enum RulesetActorType { Integration, OrganizationAdmin, RepositoryRole, Team, + DeployKey, + EnterpriseOwner, } #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "snake_case")] pub(crate) enum RulesetBypassMode { Always, + #[serde(rename = "pull_request")] PullRequest, + Exempt, } #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] @@ -636,3 +721,189 @@ pub(crate) enum RulesetOp { CreateForRepo, UpdateRuleset(i64), } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_bypass_actor_serialization() { + // Test Team actor with ID + let team_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(234)), + actor_type: RulesetActorType::Team, + bypass_mode: Some(RulesetBypassMode::Always), + }; + let json = + serde_json::to_string(&team_actor).expect("Team actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":234,"actor_type":"Team","bypass_mode":"always"}"#, + "Team actor should serialize with numeric actor_id, PascalCase actor_type, and snake_case bypass_mode" + ); + + // Test Integration actor with ID + let integration_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(123456)), + actor_type: RulesetActorType::Integration, + bypass_mode: Some(RulesetBypassMode::Always), + }; + let json = serde_json::to_string(&integration_actor) + .expect("Integration actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":123456,"actor_type":"Integration","bypass_mode":"always"}"#, + "Integration actor should serialize with numeric actor_id" + ); + + // Test OrganizationAdmin with ID 1 (per GitHub API spec) + let org_admin_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(1)), + actor_type: RulesetActorType::OrganizationAdmin, + bypass_mode: None, // Use API default + }; + let json = serde_json::to_string(&org_admin_actor) + .expect("OrganizationAdmin actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":1,"actor_type":"OrganizationAdmin"}"#, + "OrganizationAdmin should use actor_id:1 and omit bypass_mode when None" + ); + + // Test DeployKey with null actor_id (per GitHub API spec) + let deploy_key_actor = RulesetBypassActor { + actor_id: Some(ActorId::Null), + actor_type: RulesetActorType::DeployKey, + bypass_mode: Some(RulesetBypassMode::Always), + }; + let json = serde_json::to_string(&deploy_key_actor) + .expect("DeployKey actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":null,"actor_type":"DeployKey","bypass_mode":"always"}"#, + "DeployKey should serialize actor_id as null" + ); + + // Test EnterpriseOwner with None actor_id (field omitted per GitHub API spec) + let enterprise_actor = RulesetBypassActor { + actor_id: None, + actor_type: RulesetActorType::EnterpriseOwner, + bypass_mode: Some(RulesetBypassMode::Exempt), + }; + let json = serde_json::to_string(&enterprise_actor) + .expect("EnterpriseOwner actor serialization should succeed"); + assert_eq!( + json, r#"{"actor_type":"EnterpriseOwner","bypass_mode":"exempt"}"#, + "EnterpriseOwner should omit actor_id field entirely" + ); + + // Test pull_request bypass mode + let pr_actor = RulesetBypassActor { + actor_id: Some(ActorId::Id(789)), + actor_type: RulesetActorType::Team, + bypass_mode: Some(RulesetBypassMode::PullRequest), + }; + let json = serde_json::to_string(&pr_actor) + .expect("PullRequest bypass mode serialization should succeed"); + assert_eq!( + json, r#"{"actor_id":789,"actor_type":"Team","bypass_mode":"pull_request"}"#, + "PullRequest bypass mode should serialize as 'pull_request' with underscore" + ); + } + + #[test] + fn test_bypass_actor_deserialization() { + // Test deserializing from GitHub API response + let json = r#"{"actor_id":234,"actor_type":"Team","bypass_mode":"always"}"#; + let actor: RulesetBypassActor = + serde_json::from_str(json).expect("Should deserialize valid Team actor"); + assert_eq!( + actor.actor_id, + Some(ActorId::Id(234)), + "actor_id should be numeric" + ); + assert_eq!( + actor.actor_type, + RulesetActorType::Team, + "actor_type should be Team" + ); + assert_eq!( + actor.bypass_mode, + Some(RulesetBypassMode::Always), + "bypass_mode should be Always" + ); + + // Test with null actor_id (DeployKey) + // Custom deserializer ensures JSON null becomes Some(ActorId::Null), not None + let json = r#"{"actor_id":null,"actor_type":"DeployKey","bypass_mode":"always"}"#; + let actor: RulesetBypassActor = serde_json::from_str(json) + .expect("Should deserialize DeployKey actor with null actor_id"); + assert_eq!( + actor.actor_id, + Some(ActorId::Null), + "JSON null should deserialize to Some(ActorId::Null) with custom deserializer" + ); + assert_eq!(actor.actor_type, RulesetActorType::DeployKey); + + // Test with missing bypass_mode (should default to None) + let json = r#"{"actor_id":1,"actor_type":"OrganizationAdmin"}"#; + let actor: RulesetBypassActor = serde_json::from_str(json) + .expect("Should deserialize OrganizationAdmin without bypass_mode"); + assert_eq!(actor.actor_id, Some(ActorId::Id(1))); + assert_eq!( + actor.bypass_mode, None, + "bypass_mode should be None when omitted from JSON" + ); + + // Test with missing actor_id (EnterpriseOwner) + let json = r#"{"actor_type":"EnterpriseOwner","bypass_mode":"exempt"}"#; + let actor: RulesetBypassActor = serde_json::from_str(json) + .expect("Should deserialize EnterpriseOwner without actor_id"); + assert_eq!( + actor.actor_id, None, + "actor_id should be None when omitted from JSON" + ); + assert_eq!(actor.actor_type, RulesetActorType::EnterpriseOwner); + assert_eq!(actor.bypass_mode, Some(RulesetBypassMode::Exempt)); + + // Test all actor types can be deserialized + let actor_types = [ + ("Integration", RulesetActorType::Integration), + ("OrganizationAdmin", RulesetActorType::OrganizationAdmin), + ("RepositoryRole", RulesetActorType::RepositoryRole), + ("Team", RulesetActorType::Team), + ("DeployKey", RulesetActorType::DeployKey), + ("EnterpriseOwner", RulesetActorType::EnterpriseOwner), + ]; + for (type_str, expected_type) in actor_types { + let json = format!( + r#"{{"actor_id":1,"actor_type":"{}","bypass_mode":"always"}}"#, + type_str + ); + let actor: RulesetBypassActor = serde_json::from_str(&json) + .unwrap_or_else(|e| panic!("Should deserialize actor type {}: {}", type_str, e)); + assert_eq!( + actor.actor_type, expected_type, + "actor_type {} should deserialize correctly", + type_str + ); + } + + // Test all bypass modes can be deserialized + let bypass_modes = [ + ("always", RulesetBypassMode::Always), + ("pull_request", RulesetBypassMode::PullRequest), + ("exempt", RulesetBypassMode::Exempt), + ]; + for (mode_str, expected_mode) in bypass_modes { + let json = format!( + r#"{{"actor_id":1,"actor_type":"Team","bypass_mode":"{}"}}"#, + mode_str + ); + let actor: RulesetBypassActor = serde_json::from_str(&json) + .unwrap_or_else(|e| panic!("Should deserialize bypass mode {}: {}", mode_str, e)); + assert_eq!( + actor.bypass_mode, + Some(expected_mode), + "bypass_mode {} should deserialize correctly", + mode_str + ); + } + } +} diff --git a/sync-team/src/github/api/write.rs b/sync-team/src/github/api/write.rs index f24f6d741..6c89c58ff 100644 --- a/sync-team/src/github/api/write.rs +++ b/sync-team/src/github/api/write.rs @@ -89,6 +89,83 @@ impl GitHubWrite { Ok(data.organization.team.id) } + /// Resolve a team's database ID for use in rulesets + /// Returns None if the team doesn't exist in the organization + pub(crate) fn resolve_team_database_id( + &self, + org: &str, + name: &str, + ) -> anyhow::Result> { + #[derive(serde::Serialize)] + struct Params<'a> { + org: &'a str, + team: &'a str, + } + let query = " + query($org: String!, $team: String!) { + organization(login: $org) { + team(slug: $team) { + databaseId + } + } + } + "; + #[derive(serde::Deserialize)] + struct Data { + organization: Option, + } + #[derive(serde::Deserialize)] + struct Organization { + team: Option, + } + #[derive(serde::Deserialize)] + struct Team { + #[serde(rename = "databaseId")] + database_id: Option, + } + + let data: Data = self + .client + .graphql(query, Params { org, team: name }, org)?; + + Ok(data + .organization + .and_then(|org| org.team) + .and_then(|team| team.database_id)) + } + + /// Resolve a user's database ID for use in rulesets + /// Returns None if the user doesn't exist + pub(crate) fn resolve_user_database_id( + &self, + login: &str, + org: &str, + ) -> anyhow::Result> { + #[derive(serde::Serialize)] + struct Params<'a> { + login: &'a str, + } + let query = " + query($login: String!) { + user(login: $login) { + databaseId + } + } + "; + #[derive(serde::Deserialize)] + struct Data { + user: Option, + } + #[derive(serde::Deserialize)] + struct User { + #[serde(rename = "databaseId")] + database_id: Option, + } + + let data: Data = self.client.graphql(query, Params { login }, org)?; + Ok(data.user.and_then(|u| u.database_id)) + } + /// Create a team in a org pub(crate) fn create_team( &self, diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index b3bf215bf..0a03b4c33 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -368,7 +368,7 @@ impl SyncGitHub { if use_rulesets { for branch_protection in &expected_repo.branch_protections { let ruleset = construct_ruleset(expected_repo, branch_protection); - rulesets.push(ruleset); + rulesets.push((ruleset, branch_protection.clone())); } } @@ -640,12 +640,15 @@ impl SyncGitHub { 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 - if actual_ruleset.rules != expected_ruleset.rules + // We compare rules, conditions, and enforcement + // Note: We don't compare bypass_actors here since they will be populated/updated + // during apply based on the branch_protection configuration + let needs_update = actual_ruleset.rules != expected_ruleset.rules || actual_ruleset.conditions != expected_ruleset.conditions || actual_ruleset.enforcement != expected_ruleset.enforcement - { + || self.bypass_actors_need_update(&actual_ruleset, branch_protection); + + if needs_update { let id = actual_ruleset.id.unwrap_or(0); ruleset_diffs.push(RulesetDiff { name: ruleset_name, @@ -654,6 +657,7 @@ impl SyncGitHub { actual_ruleset, expected_ruleset, ), + branch_protection: branch_protection.clone(), }); } } else { @@ -661,6 +665,7 @@ impl SyncGitHub { ruleset_diffs.push(RulesetDiff { name: ruleset_name, operation: RulesetDiffOperation::Create(expected_ruleset), + branch_protection: branch_protection.clone(), }); } } @@ -668,9 +673,17 @@ impl SyncGitHub { // Any remaining rulesets in actual_rulesets_map should be deleted for (name, ruleset) in actual_rulesets_map { if let Some(id) = ruleset.id { + // For delete operations, we create a dummy branch_protection since it won't be used ruleset_diffs.push(RulesetDiff { name, operation: RulesetDiffOperation::Delete(id), + branch_protection: rust_team_data::v1::BranchProtection { + pattern: String::new(), + dismiss_stale_review: false, + mode: rust_team_data::v1::BranchProtectionMode::PrNotRequired, + allowed_merge_teams: vec![], + merge_bots: vec![], + }, }); } } @@ -689,6 +702,45 @@ impl SyncGitHub { TeamRole::Member } } + + /// Check if bypass actors need to be updated based on branch protection configuration. + /// + /// This performs a simple comparison based on the count of expected actors. + /// A more sophisticated check would require resolving team/user IDs during diff + /// creation, which we avoid for performance. The count-based check will detect + /// most cases where updates are needed (teams added/removed, bots changed). + /// + /// Note: This may produce false positives if team/bot resolution fails during + /// construct_bypass_actors (e.g., team doesn't exist), since the expected count + /// assumes all actors can be resolved. This is acceptable as it errs on the side + /// of attempting updates rather than missing required changes. + fn bypass_actors_need_update( + &self, + actual_ruleset: &api::Ruleset, + branch_protection: &rust_team_data::v1::BranchProtection, + ) -> bool { + let expected_count = + branch_protection.allowed_merge_teams.len() + branch_protection.merge_bots.len(); + + let actual_count = actual_ruleset + .bypass_actors + .as_ref() + .map_or(0, |actors| actors.len()); + + // Update needed if counts don't match or if expected but no actual actors + if expected_count != actual_count { + return true; + } + + // If both are zero, no update needed + if expected_count == 0 { + return false; + } + + // Counts match and both are non-zero - assume update not needed + // A more sophisticated check would compare actual IDs/types + false + } } fn calculate_permission_diffs( @@ -904,6 +956,86 @@ pub(crate) fn convert_branch_pattern_to_ref_pattern(pattern: &str) -> String { format!("refs/heads/{}", pattern) } +/// Construct bypass actors from branch protection settings. +/// +/// This function resolves team names and bot logins to their GitHub database IDs +/// and creates `RulesetBypassActor` entries that allow these actors to bypass +/// branch protection rules. +/// +/// # Arguments +/// * `org` - The organization name +/// * `branch_protection` - Branch protection configuration containing allowed teams and bots +/// * `github_write` - GitHub API client for resolving IDs +/// +/// # Returns +/// A vector of `RulesetBypassActor` entries, or an error if ID resolution fails +pub(crate) fn construct_bypass_actors( + org: &str, + branch_protection: &rust_team_data::v1::BranchProtection, + github_write: &api::GitHubWrite, +) -> anyhow::Result> { + let mut bypass_actors = Vec::new(); + + for team_name in &branch_protection.allowed_merge_teams { + match github_write.resolve_team_database_id(org, team_name) { + Ok(Some(actor_id)) => { + bypass_actors.push(api::RulesetBypassActor { + actor_id: Some(api::ActorId::Id(actor_id)), + actor_type: api::RulesetActorType::Team, + bypass_mode: Some(api::RulesetBypassMode::Always), + }); + } + Ok(None) => { + debug!( + "Warning: Could not resolve database ID for team '{}/{}'. \ + The team may not exist in the organization.", + org, team_name + ); + } + Err(e) => { + anyhow::bail!( + "Failed to resolve database ID for team '{}/{}': {}", + org, + team_name, + e + ); + } + } + } + + for merge_bot in &branch_protection.merge_bots { + let user_login = match merge_bot { + MergeBot::Homu => "bors", + MergeBot::RustTimer => "rust-timer", + }; + match github_write.resolve_user_database_id(user_login, org) { + Ok(Some(actor_id)) => { + bypass_actors.push(api::RulesetBypassActor { + actor_id: Some(api::ActorId::Id(actor_id)), + actor_type: api::RulesetActorType::Integration, + bypass_mode: Some(api::RulesetBypassMode::Always), + }); + } + Ok(None) => { + debug!( + "Warning: Could not resolve database ID for bot '{}'. \ + This bot may not be installed in the organization.", + user_login + ); + } + Err(e) => { + anyhow::bail!( + "Failed to resolve database ID for bot '{}': {}", + user_login, + e + ); + } + } + } + + Ok(bypass_actors) +} + /// Convert a branch protection to a ruleset with merge queue enabled pub fn construct_ruleset( _expected_repo: &rust_team_data::v1::Repo, @@ -970,8 +1102,11 @@ pub fn construct_ruleset( }, }); - // TODO: Add bypass actors based on allowed_merge_teams and merge_bots - // This would require fetching team IDs, which needs more integration + // Note: bypass_actors will be populated later when applying the diff, + // as it requires resolving team/user IDs via GitHubWrite. + // The information needed comes from: + // - branch_protection.allowed_merge_teams (team names in expected_repo.org) + // - branch_protection.merge_bots (bot user logins) api::Ruleset { id: None, @@ -979,7 +1114,7 @@ pub fn construct_ruleset( target: RulesetTarget::Branch, source_type: RulesetSourceType::Repository, enforcement: RulesetEnforcement::Active, - bypass_actors: None, + bypass_actors: None, // Will be populated when applying the diff conditions: Some(RulesetConditions { ref_name: Some(RulesetRefNameCondition { include: vec![convert_branch_pattern_to_ref_pattern( @@ -1118,7 +1253,7 @@ struct CreateRepoDiff { settings: RepoSettings, permissions: Vec, branch_protections: Vec<(String, api::BranchProtection)>, - rulesets: Vec, + rulesets: Vec<(api::Ruleset, rust_team_data::v1::BranchProtection)>, environments: Vec<(String, rust_team_data::v1::Environment)>, } @@ -1140,10 +1275,11 @@ impl CreateRepoDiff { } // Apply rulesets (in addition to branch protections if configured) - for ruleset in &self.rulesets { + for (ruleset, branch_protection) in &self.rulesets { RulesetDiff { name: ruleset.name.clone(), operation: RulesetDiffOperation::Create(ruleset.clone()), + branch_protection: branch_protection.clone(), } .apply(sync, &self.org, &self.name)?; } @@ -1196,7 +1332,7 @@ impl std::fmt::Display for CreateRepoDiff { if !rulesets.is_empty() { writeln!(f, " Rulesets:")?; - for ruleset in rulesets { + for (ruleset, _branch_protection) in rulesets { let id_str = ruleset.id.map_or("new".to_string(), |id| id.to_string()); writeln!(f, " - {} (ID: {})", ruleset.name, id_str)?; if let Some(conditions) = &ruleset.conditions @@ -1852,17 +1988,48 @@ enum BranchProtectionDiffOperation { struct RulesetDiff { name: String, operation: RulesetDiffOperation, + branch_protection: rust_team_data::v1::BranchProtection, } impl RulesetDiff { + /// Populate bypass actors for a ruleset based on branch protection configuration. + fn populate_bypass_actors( + &self, + ruleset: &mut api::Ruleset, + org: &str, + sync: &GitHubWrite, + ) -> anyhow::Result<()> { + let actors = construct_bypass_actors(org, &self.branch_protection, sync)?; + ruleset.bypass_actors = if actors.is_empty() { + None + } else { + Some(actors) + }; + Ok(()) + } + fn apply(&self, sync: &GitHubWrite, org: &str, repo_name: &str) -> anyhow::Result<()> { use api::RulesetOp; match &self.operation { RulesetDiffOperation::Create(ruleset) => { - sync.upsert_ruleset(RulesetOp::CreateForRepo, org, repo_name, ruleset)?; + let mut ruleset_with_bypass = ruleset.clone(); + self.populate_bypass_actors(&mut ruleset_with_bypass, org, sync)?; + sync.upsert_ruleset( + RulesetOp::CreateForRepo, + org, + repo_name, + &ruleset_with_bypass, + )?; } RulesetDiffOperation::Update(id, _, new_ruleset) => { - sync.upsert_ruleset(RulesetOp::UpdateRuleset(*id), org, repo_name, new_ruleset)?; + let mut ruleset_with_bypass = new_ruleset.clone(); + self.populate_bypass_actors(&mut ruleset_with_bypass, org, sync)?; + sync.upsert_ruleset( + RulesetOp::UpdateRuleset(*id), + org, + repo_name, + &ruleset_with_bypass, + )?; } RulesetDiffOperation::Delete(id) => { debug!( From 6085d8fde1d363776e528cf3efa4d776d1a7e5a6 Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Mon, 22 Dec 2025 23:43:17 +0530 Subject: [PATCH 2/5] feat: implement allowed-merge-teams allowed-merge-users --- docs/toml-schema.md | 6 +++ rust_team_data/src/v1.rs | 2 + src/schema.rs | 2 + src/static_api.rs | 1 + sync-team/src/github/mod.rs | 44 ++++++++++++++++--- sync-team/src/github/tests/test_utils.rs | 3 ++ tests/static-api/_expected/v1/repos.json | 2 + .../_expected/v1/repos/archived_repo.json | 1 + .../_expected/v1/repos/some_repo.json | 1 + 9 files changed, 56 insertions(+), 6 deletions(-) diff --git a/docs/toml-schema.md b/docs/toml-schema.md index 161afb916..08aee2657 100644 --- a/docs/toml-schema.md +++ b/docs/toml-schema.md @@ -421,6 +421,12 @@ required-approvals = 1 # in [access.teams]. # (optional) allowed-merge-teams = ["awesome-team"] +# Enable bypass for organization administrators. +# Due to GitHub API limitations, individual users cannot be granted bypass +# access directly. Instead, this grants bypass to all organization admins. +# For granular control, add users to a team and use allowed-merge-teams. +# (optional) +allowed-merge-users = ["any-username"] # Enables OrganizationAdmin bypass # Determines the merge queue bot(s) that manage pushes to this branch. # When a bot manages the queue, some other options, like # `required-approvals` and `pr-required` options are not valid. diff --git a/rust_team_data/src/v1.rs b/rust_team_data/src/v1.rs index 0f260644c..7958db215 100644 --- a/rust_team_data/src/v1.rs +++ b/rust_team_data/src/v1.rs @@ -259,6 +259,8 @@ pub struct BranchProtection { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, + #[serde(default)] + pub allowed_merge_users: Vec, pub merge_bots: Vec, } diff --git a/src/schema.rs b/src/schema.rs index 70733f301..f6644803e 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -878,6 +878,8 @@ pub(crate) struct BranchProtection { #[serde(default)] pub allowed_merge_teams: Vec, #[serde(default)] + pub allowed_merge_users: Vec, + #[serde(default)] pub merge_bots: Vec, } diff --git a/src/static_api.rs b/src/static_api.rs index c1353cbbe..8b1005d4d 100644 --- a/src/static_api.rs +++ b/src/static_api.rs @@ -62,6 +62,7 @@ impl<'a> Generator<'a> { BranchProtectionMode::PrNotRequired }, allowed_merge_teams: b.allowed_merge_teams.clone(), + allowed_merge_users: b.allowed_merge_users.clone(), merge_bots: b .merge_bots .iter() diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 0a03b4c33..3a2834a5b 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -682,6 +682,7 @@ impl SyncGitHub { dismiss_stale_review: false, mode: rust_team_data::v1::BranchProtectionMode::PrNotRequired, allowed_merge_teams: vec![], + allowed_merge_users: vec![], merge_bots: vec![], }, }); @@ -719,8 +720,13 @@ impl SyncGitHub { actual_ruleset: &api::Ruleset, branch_protection: &rust_team_data::v1::BranchProtection, ) -> bool { - let expected_count = - branch_protection.allowed_merge_teams.len() + branch_protection.merge_bots.len(); + let expected_count = branch_protection.allowed_merge_teams.len() + + if branch_protection.allowed_merge_users.is_empty() { + 0 + } else { + 1 + } // OrganizationAdmin counts as 1 + + branch_protection.merge_bots.len(); let actual_count = actual_ruleset .bypass_actors @@ -958,13 +964,21 @@ pub(crate) fn convert_branch_pattern_to_ref_pattern(pattern: &str) -> String { /// Construct bypass actors from branch protection settings. /// -/// This function resolves team names and bot logins to their GitHub database IDs -/// and creates `RulesetBypassActor` entries that allow these actors to bypass -/// branch protection rules. +/// This function resolves team names, user logins, and bot logins to their GitHub +/// database IDs and creates `RulesetBypassActor` entries that allow these actors +/// to bypass branch protection rules. +/// +/// # Actor Types +/// - Teams: Use `Team` actor type with team database ID +/// - Users: Use `RepositoryRole` actor type with user database ID +/// - Bots: Use `Integration` actor type with bot database ID /// /// # Arguments /// * `org` - The organization name -/// * `branch_protection` - Branch protection configuration containing allowed teams and bots +/// * `branch_protection` - Branch protection configuration containing: +/// - `allowed_merge_teams`: Team names to resolve +/// - `allowed_merge_users`: User logins to resolve +/// - `merge_bots`: Bot types to resolve (Homu/RustTimer) /// * `github_write` - GitHub API client for resolving IDs /// /// # Returns @@ -1003,6 +1017,24 @@ pub(crate) fn construct_bypass_actors( } } + // Note: GitHub API has limited support for individual user bypass actors. + // OrganizationAdmin with actor_id=1 grants bypass to all org admins. + // For individual users, they must be added to a team and use allowed_merge_teams. + if !branch_protection.allowed_merge_users.is_empty() { + debug!( + "Warning: Individual user bypass actors (allowed_merge_users) have limited API support. \ + Users: {:?}. Only organization admins can bypass. \ + For granular control, add users to a team and use allowed_merge_teams instead.", + branch_protection.allowed_merge_users + ); + // Add OrganizationAdmin bypass actor (applies to all org admins) + bypass_actors.push(api::RulesetBypassActor { + actor_id: Some(api::ActorId::Id(1)), + actor_type: api::RulesetActorType::OrganizationAdmin, + bypass_mode: Some(api::RulesetBypassMode::Always), + }); + } + for merge_bot in &branch_protection.merge_bots { let user_login = match merge_bot { MergeBot::Homu => "bors", diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index 36cdb83e7..f087c4506 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -448,6 +448,7 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, + allowed_merge_users, merge_bots, } = self; v1::BranchProtection { @@ -455,6 +456,7 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, + allowed_merge_users, merge_bots, } } @@ -465,6 +467,7 @@ impl BranchProtectionBuilder { mode, dismiss_stale_review: false, allowed_merge_teams: vec![], + allowed_merge_users: vec![], merge_bots: vec![], } } diff --git a/tests/static-api/_expected/v1/repos.json b/tests/static-api/_expected/v1/repos.json index ae7d5d782..2e92cd289 100644 --- a/tests/static-api/_expected/v1/repos.json +++ b/tests/static-api/_expected/v1/repos.json @@ -21,6 +21,7 @@ } }, "allowed_merge_teams": [], + "allowed_merge_users": [], "merge_bots": [] } ], @@ -62,6 +63,7 @@ "allowed_merge_teams": [ "foo" ], + "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/archived_repo.json b/tests/static-api/_expected/v1/repos/archived_repo.json index 6c00ae967..2402e2dfc 100644 --- a/tests/static-api/_expected/v1/repos/archived_repo.json +++ b/tests/static-api/_expected/v1/repos/archived_repo.json @@ -19,6 +19,7 @@ } }, "allowed_merge_teams": [], + "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/some_repo.json b/tests/static-api/_expected/v1/repos/some_repo.json index 504838674..d348a5659 100644 --- a/tests/static-api/_expected/v1/repos/some_repo.json +++ b/tests/static-api/_expected/v1/repos/some_repo.json @@ -30,6 +30,7 @@ "allowed_merge_teams": [ "foo" ], + "allowed_merge_users": [], "merge_bots": [] } ], From 20b7621628495b4a39739c7379ac52484bd81a8a Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Tue, 23 Dec 2025 00:01:38 +0530 Subject: [PATCH 3/5] feat: add logs and fix tests --- sync-team/src/github/mod.rs | 129 ++++++++++++++++------- sync-team/src/github/tests/test_utils.rs | 1 + 2 files changed, 89 insertions(+), 41 deletions(-) diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 3a2834a5b..171fdee96 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -5,6 +5,7 @@ mod tests; use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole}; use crate::Config; use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings}; +use anyhow::Context; use log::debug; use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot}; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; @@ -990,6 +991,7 @@ pub(crate) fn construct_bypass_actors( ) -> anyhow::Result> { let mut bypass_actors = Vec::new(); + // Resolve team bypass actors for team_name in &branch_protection.allowed_merge_teams { match github_write.resolve_team_database_id(org, team_name) { Ok(Some(actor_id)) => { @@ -1001,33 +1003,31 @@ pub(crate) fn construct_bypass_actors( } Ok(None) => { debug!( - "Warning: Could not resolve database ID for team '{}/{}'. \ - The team may not exist in the organization.", + "Team '{}/{}' not found; skipping bypass actor. \ + Verify the team exists in the organization.", org, team_name ); } Err(e) => { - anyhow::bail!( - "Failed to resolve database ID for team '{}/{}': {}", - org, - team_name, - e - ); + return Err(e).context(format!( + "Failed to resolve team '{}/{}' for bypass actor", + org, team_name + )); } } } - // Note: GitHub API has limited support for individual user bypass actors. - // OrganizationAdmin with actor_id=1 grants bypass to all org admins. - // For individual users, they must be added to a team and use allowed_merge_teams. + // Add OrganizationAdmin bypass actor if allowed_merge_users is specified. + // Note: GitHub API limitation - cannot add individual users as bypass actors. + // OrganizationAdmin with actor_id=1 grants bypass to ALL organization admins. + // For granular per-user control, use allowed_merge_teams with a team containing specific users. if !branch_protection.allowed_merge_users.is_empty() { debug!( - "Warning: Individual user bypass actors (allowed_merge_users) have limited API support. \ - Users: {:?}. Only organization admins can bypass. \ - For granular control, add users to a team and use allowed_merge_teams instead.", + "Using OrganizationAdmin bypass actor for allowed_merge_users: {:?}. \ + Note: This grants bypass to ALL org admins, not just specified users. \ + For per-user control, use allowed_merge_teams instead.", branch_protection.allowed_merge_users ); - // Add OrganizationAdmin bypass actor (applies to all org admins) bypass_actors.push(api::RulesetBypassActor { actor_id: Some(api::ActorId::Id(1)), actor_type: api::RulesetActorType::OrganizationAdmin, @@ -1035,6 +1035,7 @@ pub(crate) fn construct_bypass_actors( }); } + // Resolve bot integration bypass actors for merge_bot in &branch_protection.merge_bots { let user_login = match merge_bot { MergeBot::Homu => "bors", @@ -1050,17 +1051,16 @@ pub(crate) fn construct_bypass_actors( } Ok(None) => { debug!( - "Warning: Could not resolve database ID for bot '{}'. \ - This bot may not be installed in the organization.", - user_login + "Bot '{}' not found in organization '{}'; skipping bypass actor. \ + Verify the bot is installed and has access.", + user_login, org ); } Err(e) => { - anyhow::bail!( - "Failed to resolve database ID for bot '{}': {}", - user_login, - e - ); + return Err(e).context(format!( + "Failed to resolve bot '{}' for bypass actor", + user_login + )); } } } @@ -1364,21 +1364,9 @@ impl std::fmt::Display for CreateRepoDiff { if !rulesets.is_empty() { writeln!(f, " Rulesets:")?; - for (ruleset, _branch_protection) in rulesets { - let id_str = ruleset.id.map_or("new".to_string(), |id| id.to_string()); - writeln!(f, " - {} (ID: {})", ruleset.name, id_str)?; - if let Some(conditions) = &ruleset.conditions - && let Some(ref_name) = &conditions.ref_name - { - writeln!(f, " Branches: {}", ref_name.include.join(", "))?; - } - if ruleset - .rules - .iter() - .any(|r| matches!(r, api::RulesetRule::MergeQueue { .. })) - { - writeln!(f, " Merge Queue: Enabled")?; - } + for (ruleset, branch_protection) in rulesets { + writeln!(f, " {}", ruleset.name)?; + log_ruleset(ruleset, None, Some(branch_protection), &mut f)?; } } @@ -1814,6 +1802,7 @@ fn log_branch_protection( fn log_ruleset( current: &api::Ruleset, new: Option<&api::Ruleset>, + branch_protection: Option<&rust_team_data::v1::BranchProtection>, mut result: impl Write, ) -> std::fmt::Result { let is_create = new.is_none(); @@ -1874,7 +1863,28 @@ fn log_ruleset( && !bypass_actors.is_empty() { let new_bypass = new.and_then(|n| n.bypass_actors.as_ref()); - log!("Bypass Actors", bypass_actors, new_bypass); + if new_bypass.is_some() { + log!("Bypass Actors", bypass_actors, new_bypass); + } else { + writeln!(result, " Bypass Actors:")?; + for actor in bypass_actors { + let actor_id_str = match &actor.actor_id { + Some(api::ActorId::Id(id)) => id.to_string(), + Some(api::ActorId::Null) => "null".to_string(), + None => "omitted".to_string(), + }; + let mode_str = actor.bypass_mode.as_ref().map_or("default", |m| match m { + api::RulesetBypassMode::Always => "always", + api::RulesetBypassMode::PullRequest => "pull_request", + api::RulesetBypassMode::Exempt => "exempt", + }); + writeln!( + result, + " - Type: {:?}, ID: {}, Mode: {}", + actor.actor_type, actor_id_str, mode_str + )?; + } + } } // Log individual rules @@ -2002,6 +2012,39 @@ fn log_ruleset( } } + // Log expected bypass actors if branch protection config is provided + if let Some(bp) = branch_protection + && (!bp.allowed_merge_teams.is_empty() + || !bp.allowed_merge_users.is_empty() + || !bp.merge_bots.is_empty()) + { + writeln!(result, " Expected Bypass Actors:")?; + + for team in &bp.allowed_merge_teams { + writeln!(result, " - Team: {} (Mode: always)", team)?; + } + + if !bp.allowed_merge_users.is_empty() { + writeln!( + result, + " - OrganizationAdmin: ID=1 (Mode: always) [Note: Grants bypass to all org admins, requested users: {}]", + bp.allowed_merge_users.join(", ") + )?; + } + + for bot in &bp.merge_bots { + let bot_name = match bot { + MergeBot::Homu => "bors", + MergeBot::RustTimer => "rust-timer", + }; + writeln!( + result, + " - Integration: {} (Mode: always)", + bot_name + )?; + } + } + if !is_create && !logged { writeln!(result, " No changes")?; } @@ -2080,8 +2123,12 @@ impl std::fmt::Display for RulesetDiff { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, " {}", self.name)?; match &self.operation { - RulesetDiffOperation::Create(ruleset) => log_ruleset(ruleset, None, f), - RulesetDiffOperation::Update(_, old, new) => log_ruleset(old, Some(new), f), + RulesetDiffOperation::Create(ruleset) => { + log_ruleset(ruleset, None, Some(&self.branch_protection), &mut *f) + } + RulesetDiffOperation::Update(_, old, new) => { + log_ruleset(old, Some(new), Some(&self.branch_protection), &mut *f) + } RulesetDiffOperation::Delete(_) => { writeln!(f, " Deleting ruleset") } diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index f087c4506..e13de38e0 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -424,6 +424,7 @@ pub struct BranchProtectionBuilder { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, + pub allowed_merge_users: Vec, pub merge_bots: Vec, } From 1d8c52b60dba58c6de14b7320724d00926f0a8bc Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Tue, 23 Dec 2025 10:12:17 +0530 Subject: [PATCH 4/5] revert: allowed merged users --- docs/toml-schema.md | 6 --- rust_team_data/src/v1.rs | 2 - src/schema.rs | 2 - src/static_api.rs | 1 - sync-team/src/github/mod.rs | 49 +++++-------------- sync-team/src/github/tests/test_utils.rs | 4 -- tests/static-api/_expected/v1/repos.json | 2 - .../_expected/v1/repos/archived_repo.json | 1 - .../_expected/v1/repos/some_repo.json | 1 - 9 files changed, 12 insertions(+), 56 deletions(-) diff --git a/docs/toml-schema.md b/docs/toml-schema.md index 08aee2657..161afb916 100644 --- a/docs/toml-schema.md +++ b/docs/toml-schema.md @@ -421,12 +421,6 @@ required-approvals = 1 # in [access.teams]. # (optional) allowed-merge-teams = ["awesome-team"] -# Enable bypass for organization administrators. -# Due to GitHub API limitations, individual users cannot be granted bypass -# access directly. Instead, this grants bypass to all organization admins. -# For granular control, add users to a team and use allowed-merge-teams. -# (optional) -allowed-merge-users = ["any-username"] # Enables OrganizationAdmin bypass # Determines the merge queue bot(s) that manage pushes to this branch. # When a bot manages the queue, some other options, like # `required-approvals` and `pr-required` options are not valid. diff --git a/rust_team_data/src/v1.rs b/rust_team_data/src/v1.rs index 7958db215..0f260644c 100644 --- a/rust_team_data/src/v1.rs +++ b/rust_team_data/src/v1.rs @@ -259,8 +259,6 @@ pub struct BranchProtection { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, - #[serde(default)] - pub allowed_merge_users: Vec, pub merge_bots: Vec, } diff --git a/src/schema.rs b/src/schema.rs index f6644803e..70733f301 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -878,8 +878,6 @@ pub(crate) struct BranchProtection { #[serde(default)] pub allowed_merge_teams: Vec, #[serde(default)] - pub allowed_merge_users: Vec, - #[serde(default)] pub merge_bots: Vec, } diff --git a/src/static_api.rs b/src/static_api.rs index 8b1005d4d..c1353cbbe 100644 --- a/src/static_api.rs +++ b/src/static_api.rs @@ -62,7 +62,6 @@ impl<'a> Generator<'a> { BranchProtectionMode::PrNotRequired }, allowed_merge_teams: b.allowed_merge_teams.clone(), - allowed_merge_users: b.allowed_merge_users.clone(), merge_bots: b .merge_bots .iter() diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 171fdee96..f26639b7c 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -683,7 +683,6 @@ impl SyncGitHub { dismiss_stale_review: false, mode: rust_team_data::v1::BranchProtectionMode::PrNotRequired, allowed_merge_teams: vec![], - allowed_merge_users: vec![], merge_bots: vec![], }, }); @@ -721,13 +720,8 @@ impl SyncGitHub { actual_ruleset: &api::Ruleset, branch_protection: &rust_team_data::v1::BranchProtection, ) -> bool { - let expected_count = branch_protection.allowed_merge_teams.len() - + if branch_protection.allowed_merge_users.is_empty() { - 0 - } else { - 1 - } // OrganizationAdmin counts as 1 - + branch_protection.merge_bots.len(); + let expected_count = + branch_protection.allowed_merge_teams.len() + branch_protection.merge_bots.len(); let actual_count = actual_ruleset .bypass_actors @@ -1017,29 +1011,16 @@ pub(crate) fn construct_bypass_actors( } } - // Add OrganizationAdmin bypass actor if allowed_merge_users is specified. - // Note: GitHub API limitation - cannot add individual users as bypass actors. - // OrganizationAdmin with actor_id=1 grants bypass to ALL organization admins. - // For granular per-user control, use allowed_merge_teams with a team containing specific users. - if !branch_protection.allowed_merge_users.is_empty() { - debug!( - "Using OrganizationAdmin bypass actor for allowed_merge_users: {:?}. \ - Note: This grants bypass to ALL org admins, not just specified users. \ - For per-user control, use allowed_merge_teams instead.", - branch_protection.allowed_merge_users - ); - bypass_actors.push(api::RulesetBypassActor { - actor_id: Some(api::ActorId::Id(1)), - actor_type: api::RulesetActorType::OrganizationAdmin, - bypass_mode: Some(api::RulesetBypassMode::Always), - }); - } - // Resolve bot integration bypass actors for merge_bot in &branch_protection.merge_bots { let user_login = match merge_bot { MergeBot::Homu => "bors", MergeBot::RustTimer => "rust-timer", + MergeBot::Bors => { + // Bors uses a GitHub app, which is not configured through team (it is set manually) + // Its bypass actor will be roundtripped by sync-team. + continue; + } }; match github_write.resolve_user_database_id(user_login, org) { Ok(Some(actor_id)) => { @@ -2014,9 +1995,7 @@ fn log_ruleset( // Log expected bypass actors if branch protection config is provided if let Some(bp) = branch_protection - && (!bp.allowed_merge_teams.is_empty() - || !bp.allowed_merge_users.is_empty() - || !bp.merge_bots.is_empty()) + && (!bp.allowed_merge_teams.is_empty() || !bp.merge_bots.is_empty()) { writeln!(result, " Expected Bypass Actors:")?; @@ -2024,18 +2003,14 @@ fn log_ruleset( writeln!(result, " - Team: {} (Mode: always)", team)?; } - if !bp.allowed_merge_users.is_empty() { - writeln!( - result, - " - OrganizationAdmin: ID=1 (Mode: always) [Note: Grants bypass to all org admins, requested users: {}]", - bp.allowed_merge_users.join(", ") - )?; - } - for bot in &bp.merge_bots { let bot_name = match bot { MergeBot::Homu => "bors", MergeBot::RustTimer => "rust-timer", + MergeBot::Bors => { + // Bors uses a GitHub app, configured manually - skip logging + continue; + } }; writeln!( result, diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index e13de38e0..36cdb83e7 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -424,7 +424,6 @@ pub struct BranchProtectionBuilder { pub dismiss_stale_review: bool, pub mode: BranchProtectionMode, pub allowed_merge_teams: Vec, - pub allowed_merge_users: Vec, pub merge_bots: Vec, } @@ -449,7 +448,6 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, - allowed_merge_users, merge_bots, } = self; v1::BranchProtection { @@ -457,7 +455,6 @@ impl BranchProtectionBuilder { dismiss_stale_review, mode, allowed_merge_teams, - allowed_merge_users, merge_bots, } } @@ -468,7 +465,6 @@ impl BranchProtectionBuilder { mode, dismiss_stale_review: false, allowed_merge_teams: vec![], - allowed_merge_users: vec![], merge_bots: vec![], } } diff --git a/tests/static-api/_expected/v1/repos.json b/tests/static-api/_expected/v1/repos.json index 2e92cd289..ae7d5d782 100644 --- a/tests/static-api/_expected/v1/repos.json +++ b/tests/static-api/_expected/v1/repos.json @@ -21,7 +21,6 @@ } }, "allowed_merge_teams": [], - "allowed_merge_users": [], "merge_bots": [] } ], @@ -63,7 +62,6 @@ "allowed_merge_teams": [ "foo" ], - "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/archived_repo.json b/tests/static-api/_expected/v1/repos/archived_repo.json index 2402e2dfc..6c00ae967 100644 --- a/tests/static-api/_expected/v1/repos/archived_repo.json +++ b/tests/static-api/_expected/v1/repos/archived_repo.json @@ -19,7 +19,6 @@ } }, "allowed_merge_teams": [], - "allowed_merge_users": [], "merge_bots": [] } ], diff --git a/tests/static-api/_expected/v1/repos/some_repo.json b/tests/static-api/_expected/v1/repos/some_repo.json index d348a5659..504838674 100644 --- a/tests/static-api/_expected/v1/repos/some_repo.json +++ b/tests/static-api/_expected/v1/repos/some_repo.json @@ -30,7 +30,6 @@ "allowed_merge_teams": [ "foo" ], - "allowed_merge_users": [], "merge_bots": [] } ], From 03305752bde102a8d6399d131e47f3f17c7aa8aa Mon Sep 17 00:00:00 2001 From: Mustaque Ahmed Date: Wed, 7 Jan 2026 10:01:21 +0530 Subject: [PATCH 5/5] refactor: remove unused enum values --- sync-team/src/github/api/mod.rs | 203 +++++--------------------------- sync-team/src/github/mod.rs | 13 +- 2 files changed, 34 insertions(+), 182 deletions(-) diff --git a/sync-team/src/github/api/mod.rs b/sync-team/src/github/api/mod.rs index 7005b6960..9052a1e72 100644 --- a/sync-team/src/github/api/mod.rs +++ b/sync-team/src/github/api/mod.rs @@ -522,100 +522,22 @@ pub(crate) enum RulesetEnforcement { #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub(crate) struct RulesetBypassActor { /// The ID of the actor that can bypass a ruleset. - /// - Required for Integration, RepositoryRole, and Team actor types (use Some(ActorId::Id(n))) - /// - Must be 1 for OrganizationAdmin (use Some(ActorId::Id(1))) - /// - Must be null for DeployKey (use Some(ActorId::Null)) - /// - Omitted for EnterpriseOwner (use None) - #[serde( - skip_serializing_if = "Option::is_none", - default, - deserialize_with = "deserialize_actor_id" - )] - pub(crate) actor_id: Option, + /// Required for Team and Integration actor types. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) actor_id: Option, pub(crate) actor_type: RulesetActorType, /// The bypass mode for the actor. Defaults to "always" per GitHub API. - /// - always: Actor can always bypass - /// - pull_request: Actor can only bypass on pull requests (not applicable for DeployKey) - /// - exempt: Rules won't run for actor, no audit entry created #[serde(skip_serializing_if = "Option::is_none")] pub(crate) bypass_mode: Option, } -/// Custom deserializer for actor_id that distinguishes between null and missing -fn deserialize_actor_id<'de, D>(deserializer: D) -> Result, D::Error> -where - D: serde::Deserializer<'de>, -{ - use serde::{Deserialize, de}; - - struct ActorIdVisitor; - - impl<'de> de::Visitor<'de> for ActorIdVisitor { - type Value = Option; - - fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { - formatter.write_str("an integer, null, or missing field") - } - - fn visit_none(self) -> Result - where - E: de::Error, - { - Ok(Some(ActorId::Null)) - } - - fn visit_some(self, deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let id = i64::deserialize(deserializer)?; - Ok(Some(ActorId::Id(id))) - } - - fn visit_unit(self) -> Result - where - E: de::Error, - { - Ok(Some(ActorId::Null)) - } - - fn visit_i64(self, v: i64) -> Result - where - E: de::Error, - { - Ok(Some(ActorId::Id(v))) - } - - fn visit_u64(self, v: u64) -> Result - where - E: de::Error, - { - Ok(Some(ActorId::Id(v as i64))) - } - } - - deserializer.deserialize_option(ActorIdVisitor) -} - -/// Represents the actor_id field which can be a number or null -#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] -#[serde(untagged)] -pub(crate) enum ActorId { - /// A specific actor ID (used for Integration, RepositoryRole, Team, OrganizationAdmin) - Id(i64), - /// Explicitly null (required for DeployKey) - Null, -} - #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "PascalCase")] pub(crate) enum RulesetActorType { + /// GitHub App integration Integration, - OrganizationAdmin, - RepositoryRole, + /// GitHub Team Team, - DeployKey, - EnterpriseOwner, } #[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] @@ -730,7 +652,7 @@ mod tests { fn test_bypass_actor_serialization() { // Test Team actor with ID let team_actor = RulesetBypassActor { - actor_id: Some(ActorId::Id(234)), + actor_id: Some(234), actor_type: RulesetActorType::Team, bypass_mode: Some(RulesetBypassMode::Always), }; @@ -743,7 +665,7 @@ mod tests { // Test Integration actor with ID let integration_actor = RulesetBypassActor { - actor_id: Some(ActorId::Id(123456)), + actor_id: Some(123456), actor_type: RulesetActorType::Integration, bypass_mode: Some(RulesetBypassMode::Always), }; @@ -754,48 +676,22 @@ mod tests { "Integration actor should serialize with numeric actor_id" ); - // Test OrganizationAdmin with ID 1 (per GitHub API spec) - let org_admin_actor = RulesetBypassActor { - actor_id: Some(ActorId::Id(1)), - actor_type: RulesetActorType::OrganizationAdmin, - bypass_mode: None, // Use API default - }; - let json = serde_json::to_string(&org_admin_actor) - .expect("OrganizationAdmin actor serialization should succeed"); - assert_eq!( - json, r#"{"actor_id":1,"actor_type":"OrganizationAdmin"}"#, - "OrganizationAdmin should use actor_id:1 and omit bypass_mode when None" - ); - - // Test DeployKey with null actor_id (per GitHub API spec) - let deploy_key_actor = RulesetBypassActor { - actor_id: Some(ActorId::Null), - actor_type: RulesetActorType::DeployKey, - bypass_mode: Some(RulesetBypassMode::Always), - }; - let json = serde_json::to_string(&deploy_key_actor) - .expect("DeployKey actor serialization should succeed"); - assert_eq!( - json, r#"{"actor_id":null,"actor_type":"DeployKey","bypass_mode":"always"}"#, - "DeployKey should serialize actor_id as null" - ); - - // Test EnterpriseOwner with None actor_id (field omitted per GitHub API spec) - let enterprise_actor = RulesetBypassActor { + // Test with None actor_id (field omitted) + let actor_no_id = RulesetBypassActor { actor_id: None, - actor_type: RulesetActorType::EnterpriseOwner, - bypass_mode: Some(RulesetBypassMode::Exempt), + actor_type: RulesetActorType::Team, + bypass_mode: Some(RulesetBypassMode::Always), }; - let json = serde_json::to_string(&enterprise_actor) - .expect("EnterpriseOwner actor serialization should succeed"); + let json = serde_json::to_string(&actor_no_id) + .expect("Actor without ID serialization should succeed"); assert_eq!( - json, r#"{"actor_type":"EnterpriseOwner","bypass_mode":"exempt"}"#, - "EnterpriseOwner should omit actor_id field entirely" + json, r#"{"actor_type":"Team","bypass_mode":"always"}"#, + "Actor without ID should omit actor_id field" ); // Test pull_request bypass mode let pr_actor = RulesetBypassActor { - actor_id: Some(ActorId::Id(789)), + actor_id: Some(789), actor_type: RulesetActorType::Team, bypass_mode: Some(RulesetBypassMode::PullRequest), }; @@ -809,15 +705,11 @@ mod tests { #[test] fn test_bypass_actor_deserialization() { - // Test deserializing from GitHub API response + // Test deserializing Team actor from GitHub API response let json = r#"{"actor_id":234,"actor_type":"Team","bypass_mode":"always"}"#; let actor: RulesetBypassActor = serde_json::from_str(json).expect("Should deserialize valid Team actor"); - assert_eq!( - actor.actor_id, - Some(ActorId::Id(234)), - "actor_id should be numeric" - ); + assert_eq!(actor.actor_id, Some(234), "actor_id should be numeric"); assert_eq!( actor.actor_type, RulesetActorType::Team, @@ -829,62 +721,23 @@ mod tests { "bypass_mode should be Always" ); - // Test with null actor_id (DeployKey) - // Custom deserializer ensures JSON null becomes Some(ActorId::Null), not None - let json = r#"{"actor_id":null,"actor_type":"DeployKey","bypass_mode":"always"}"#; - let actor: RulesetBypassActor = serde_json::from_str(json) - .expect("Should deserialize DeployKey actor with null actor_id"); - assert_eq!( - actor.actor_id, - Some(ActorId::Null), - "JSON null should deserialize to Some(ActorId::Null) with custom deserializer" - ); - assert_eq!(actor.actor_type, RulesetActorType::DeployKey); + // Test deserializing Integration actor + let json = r#"{"actor_id":456,"actor_type":"Integration","bypass_mode":"always"}"#; + let actor: RulesetBypassActor = + serde_json::from_str(json).expect("Should deserialize valid Integration actor"); + assert_eq!(actor.actor_id, Some(456)); + assert_eq!(actor.actor_type, RulesetActorType::Integration); // Test with missing bypass_mode (should default to None) - let json = r#"{"actor_id":1,"actor_type":"OrganizationAdmin"}"#; - let actor: RulesetBypassActor = serde_json::from_str(json) - .expect("Should deserialize OrganizationAdmin without bypass_mode"); - assert_eq!(actor.actor_id, Some(ActorId::Id(1))); + let json = r#"{"actor_id":1,"actor_type":"Team"}"#; + let actor: RulesetBypassActor = + serde_json::from_str(json).expect("Should deserialize Team without bypass_mode"); + assert_eq!(actor.actor_id, Some(1)); assert_eq!( actor.bypass_mode, None, "bypass_mode should be None when omitted from JSON" ); - // Test with missing actor_id (EnterpriseOwner) - let json = r#"{"actor_type":"EnterpriseOwner","bypass_mode":"exempt"}"#; - let actor: RulesetBypassActor = serde_json::from_str(json) - .expect("Should deserialize EnterpriseOwner without actor_id"); - assert_eq!( - actor.actor_id, None, - "actor_id should be None when omitted from JSON" - ); - assert_eq!(actor.actor_type, RulesetActorType::EnterpriseOwner); - assert_eq!(actor.bypass_mode, Some(RulesetBypassMode::Exempt)); - - // Test all actor types can be deserialized - let actor_types = [ - ("Integration", RulesetActorType::Integration), - ("OrganizationAdmin", RulesetActorType::OrganizationAdmin), - ("RepositoryRole", RulesetActorType::RepositoryRole), - ("Team", RulesetActorType::Team), - ("DeployKey", RulesetActorType::DeployKey), - ("EnterpriseOwner", RulesetActorType::EnterpriseOwner), - ]; - for (type_str, expected_type) in actor_types { - let json = format!( - r#"{{"actor_id":1,"actor_type":"{}","bypass_mode":"always"}}"#, - type_str - ); - let actor: RulesetBypassActor = serde_json::from_str(&json) - .unwrap_or_else(|e| panic!("Should deserialize actor type {}: {}", type_str, e)); - assert_eq!( - actor.actor_type, expected_type, - "actor_type {} should deserialize correctly", - type_str - ); - } - // Test all bypass modes can be deserialized let bypass_modes = [ ("always", RulesetBypassMode::Always), diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index f26639b7c..4fe60813d 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -990,7 +990,7 @@ pub(crate) fn construct_bypass_actors( match github_write.resolve_team_database_id(org, team_name) { Ok(Some(actor_id)) => { bypass_actors.push(api::RulesetBypassActor { - actor_id: Some(api::ActorId::Id(actor_id)), + actor_id: Some(actor_id), actor_type: api::RulesetActorType::Team, bypass_mode: Some(api::RulesetBypassMode::Always), }); @@ -1025,7 +1025,7 @@ pub(crate) fn construct_bypass_actors( match github_write.resolve_user_database_id(user_login, org) { Ok(Some(actor_id)) => { bypass_actors.push(api::RulesetBypassActor { - actor_id: Some(api::ActorId::Id(actor_id)), + actor_id: Some(actor_id), actor_type: api::RulesetActorType::Integration, bypass_mode: Some(api::RulesetBypassMode::Always), }); @@ -1849,11 +1849,10 @@ fn log_ruleset( } else { writeln!(result, " Bypass Actors:")?; for actor in bypass_actors { - let actor_id_str = match &actor.actor_id { - Some(api::ActorId::Id(id)) => id.to_string(), - Some(api::ActorId::Null) => "null".to_string(), - None => "omitted".to_string(), - }; + let actor_id_str = actor + .actor_id + .map(|id| id.to_string()) + .unwrap_or_else(|| "none".to_string()); let mode_str = actor.bypass_mode.as_ref().map_or("default", |m| match m { api::RulesetBypassMode::Always => "always", api::RulesetBypassMode::PullRequest => "pull_request",