From b8049db0e795133961c43fe5820ed7420f3f3a71 Mon Sep 17 00:00:00 2001 From: Rohit Dandamudi Date: Wed, 4 Jun 2025 09:06:18 +0200 Subject: [PATCH] Sync organization members --- sync-team/src/github/api/read.rs | 23 +++++ sync-team/src/github/api/write.rs | 12 +++ sync-team/src/github/mod.rs | 103 ++++++++++++++++++++++- sync-team/src/github/tests/mod.rs | 22 +++++ sync-team/src/github/tests/test_utils.rs | 33 +++++++- 5 files changed, 188 insertions(+), 5 deletions(-) diff --git a/sync-team/src/github/api/read.rs b/sync-team/src/github/api/read.rs index e68a191f8..9a4031ec2 100644 --- a/sync-team/src/github/api/read.rs +++ b/sync-team/src/github/api/read.rs @@ -15,6 +15,9 @@ pub(crate) trait GithubRead { /// Get the owners of an org fn org_owners(&self, org: &str) -> anyhow::Result>; + /// Get the members of an org + fn org_members(&self, org: &str) -> anyhow::Result>; + /// Get all teams associated with a org /// /// Returns a list of tuples of team name and slug @@ -119,6 +122,26 @@ impl GithubRead for GitHubApiRead { Ok(owners) } + fn org_members(&self, org: &str) -> anyhow::Result> { + #[derive(serde::Deserialize, Eq, PartialEq, Hash)] + struct User { + id: u64, + login: String, + } + let mut members = HashMap::new(); + self.client.rest_paginated( + &Method::GET, + &GitHubUrl::orgs(org, "members")?, + |resp: Vec| { + for user in resp { + members.insert(user.id, user.login); + } + Ok(()) + }, + )?; + Ok(members) + } + fn org_teams(&self, org: &str) -> anyhow::Result> { let mut teams = Vec::new(); diff --git a/sync-team/src/github/api/write.rs b/sync-team/src/github/api/write.rs index 39cb5cc03..75b414148 100644 --- a/sync-team/src/github/api/write.rs +++ b/sync-team/src/github/api/write.rs @@ -344,6 +344,18 @@ impl GitHubWrite { Ok(()) } + /// Remove a member from an org + pub(crate) fn remove_gh_member_from_org(&self, org: &str, user: &str) -> anyhow::Result<()> { + debug!("Removing user {user} from org {org}"); + if !self.dry_run { + let method = Method::DELETE; + let url = GitHubUrl::orgs(org, &format!("members/{user}"))?; + let resp = self.client.req(method.clone(), &url)?.send()?; + allow_not_found(resp, method, url.url())?; + } + Ok(()) + } + /// Remove a collaborator from a repo pub(crate) fn remove_collaborator_from_repo( &self, diff --git a/sync-team/src/github/mod.rs b/sync-team/src/github/mod.rs index 3f563e621..177e1e177 100644 --- a/sync-team/src/github/mod.rs +++ b/sync-team/src/github/mod.rs @@ -6,7 +6,7 @@ use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole}; use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings}; use log::debug; use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot}; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Write; pub(crate) use self::api::{GitHubApiRead, GitHubWrite, HttpClient}; @@ -31,6 +31,7 @@ struct SyncGitHub { repos: Vec, usernames_cache: HashMap, org_owners: HashMap>, + org_members: HashMap>, } impl SyncGitHub { @@ -60,9 +61,11 @@ impl SyncGitHub { .collect::>(); let mut org_owners = HashMap::new(); + let mut org_members = HashMap::new(); for org in &orgs { org_owners.insert((*org).to_string(), github.org_owners(org)?); + org_members.insert((*org).to_string(), github.org_members(org)?); } Ok(SyncGitHub { @@ -71,19 +74,74 @@ impl SyncGitHub { repos, usernames_cache, org_owners, + org_members, }) } pub(crate) fn diff_all(&self) -> anyhow::Result { let team_diffs = self.diff_teams()?; let repo_diffs = self.diff_repos()?; + let org_membership_diffs = self.diff_org_memberships()?; Ok(Diff { team_diffs, repo_diffs, + org_membership_diffs, }) } + // Collect all org members from the respective teams + fn get_org_members_from_teams(&self) -> HashMap> { + let mut org_team_members: HashMap> = HashMap::new(); + + for team in &self.teams { + if let Some(gh) = &team.github { + for toml_gh_team in &gh.teams { + org_team_members + .entry(toml_gh_team.org.clone()) + .or_default() + .extend(toml_gh_team.members.iter().copied()); + } + } + } + org_team_members + } + + // Diff organization memberships between TOML teams and GitHub + fn diff_org_memberships(&self) -> anyhow::Result> { + let toml_org_team_members = self.get_org_members_from_teams(); + + let mut org_diffs: BTreeMap = BTreeMap::new(); + + for (org, toml_members) in toml_org_team_members { + let Some(gh_org_members) = self.org_members.get(&org) else { + panic!("GitHub organization {org} not found"); + }; + + // Remove all members that are in TOML teams + let mut members_to_remove = gh_org_members.clone(); + for member in toml_members { + members_to_remove.remove(&member); + } + + // The rest are members that should be removed + if !members_to_remove.is_empty() { + let mut members_to_remove: Vec = members_to_remove.into_values().collect(); + members_to_remove.sort(); + + org_diffs.insert( + org.clone(), + OrgMembershipDiff { + org, + members_to_remove, + }, + ); + } + } + + Ok(org_diffs.into_values().collect()) + } + fn diff_teams(&self) -> anyhow::Result> { let mut diffs = Vec::new(); let mut unseen_github_teams = HashMap::new(); @@ -584,6 +642,7 @@ const BOTS_TEAMS: &[&str] = &["bors", "highfive", "rfcbot", "bots"]; pub(crate) struct Diff { team_diffs: Vec, repo_diffs: Vec, + org_membership_diffs: Vec, } impl Diff { @@ -595,12 +654,17 @@ impl Diff { for repo_diff in self.repo_diffs { repo_diff.apply(sync)?; } + for org_diff in self.org_membership_diffs { + org_diff.apply(sync)?; + } Ok(()) } pub(crate) fn is_empty(&self) -> bool { - self.team_diffs.is_empty() && self.repo_diffs.is_empty() + self.team_diffs.is_empty() + && self.repo_diffs.is_empty() + && self.org_membership_diffs.is_empty() } } @@ -620,6 +684,13 @@ impl std::fmt::Display for Diff { } } + if !&self.org_membership_diffs.is_empty() { + writeln!(f, "💻 Org membership Diffs:")?; + for org_diff in &self.org_membership_diffs { + write!(f, "{org_diff}")?; + } + } + Ok(()) } } @@ -655,6 +726,34 @@ impl std::fmt::Display for RepoDiff { } } +#[derive(Debug)] +struct OrgMembershipDiff { + org: OrgName, + members_to_remove: Vec, +} + +impl OrgMembershipDiff { + fn apply(self, sync: &GitHubWrite) -> anyhow::Result<()> { + for member in &self.members_to_remove { + sync.remove_gh_member_from_org(&self.org, &member)?; + } + + Ok(()) + } +} + +impl std::fmt::Display for OrgMembershipDiff { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if !self.members_to_remove.is_empty() { + writeln!(f, "❌ Removing the following members from `{}`:", self.org)?; + for member in &self.members_to_remove { + writeln!(f, " - {member}",)?; + } + } + Ok(()) + } +} + #[derive(Debug)] struct CreateRepoDiff { org: String, diff --git a/sync-team/src/github/tests/mod.rs b/sync-team/src/github/tests/mod.rs index f74a8ae70..25b79fffc 100644 --- a/sync-team/src/github/tests/mod.rs +++ b/sync-team/src/github/tests/mod.rs @@ -97,6 +97,28 @@ fn team_dont_add_member_if_invitation_is_pending() { insta::assert_debug_snapshot!(team_diff, @"[]"); } +#[test] +fn remove_org_members() { + let mut model = DataModel::default(); + let user = model.create_user("sakura"); + model.create_team(TeamData::new("team-1").gh_team("rust-lang", "members-gh", &[user])); + let mut gh = model.gh_model(); + gh.add_member("rust-lang", "martin"); + + let gh_org_diff = model.diff_org_membership(gh); + + insta::assert_debug_snapshot!(gh_org_diff, @r#" + [ + OrgMembershipDiff { + org: "rust-lang", + members_to_remove: [ + "martin", + ], + }, + ] + "#); +} + #[test] fn team_remove_member() { let mut model = DataModel::default(); diff --git a/sync-team/src/github/tests/test_utils.rs b/sync-team/src/github/tests/test_utils.rs index a5bab6d74..ccafb27af 100644 --- a/sync-team/src/github/tests/test_utils.rs +++ b/sync-team/src/github/tests/test_utils.rs @@ -10,7 +10,8 @@ use crate::github::api::{ BranchProtection, GithubRead, Repo, RepoTeam, RepoUser, Team, TeamMember, TeamPrivacy, TeamRole, }; use crate::github::{ - RepoDiff, SyncGitHub, TeamDiff, api, construct_branch_protection, convert_permission, + OrgMembershipDiff, RepoDiff, SyncGitHub, TeamDiff, api, construct_branch_protection, + convert_permission, }; pub const DEFAULT_ORG: &str = "rust-lang"; @@ -105,7 +106,13 @@ impl DataModel { slug: gh_team.name.clone(), }); - org.members.extend(gh_team.members.iter().copied()); + org.members.extend( + gh_team + .members + .iter() + .copied() + .map(|user_id| (user_id, users[&user_id].clone())), + ); } } @@ -168,6 +175,12 @@ impl DataModel { GithubMock { users, orgs } } + pub fn diff_org_membership(&self, github: GithubMock) -> Vec { + self.create_sync(github) + .diff_org_memberships() + .expect("Cannot diff org membership") + } + pub fn diff_teams(&self, github: GithubMock) -> Vec { self.create_sync(github) .diff_teams() @@ -416,6 +429,16 @@ pub struct GithubMock { } impl GithubMock { + pub fn add_member(&mut self, org: &str, username: &str) { + let user_id = self.users.len() as UserId; + self.users.insert(user_id, username.to_string()); + self.orgs + .get_mut(org) + .unwrap() + .members + .insert((user_id, username.to_string())); + } + pub fn add_invitation(&mut self, org: &str, repo: &str, user: &str) { self.get_org_mut(org) .team_invitations @@ -455,6 +478,10 @@ impl GithubRead for GithubMock { Ok(self.get_org(org).owners.iter().copied().collect()) } + fn org_members(&self, org: &str) -> anyhow::Result> { + Ok(self.get_org(org).members.iter().cloned().collect()) + } + fn org_teams(&self, org: &str) -> anyhow::Result> { Ok(self .get_org(org) @@ -547,7 +574,7 @@ impl GithubRead for GithubMock { #[derive(Default)] struct GithubOrg { - members: BTreeSet, + members: BTreeSet<(UserId, String)>, owners: BTreeSet, teams: Vec, // Team name -> list of invited users