add org members allow list

Author: marcoieni

config.toml

@@ -15,6 +15,12 @@ allowed-github-orgs = [
     "rust-analyzer",
 ]
 
+# GitHub organizations where member management is independent
+# and should not be automatically synchronized
+independent-github-orgs = [
+    "rust-embedded",
+]
+
 permissions-bors-repos = [
     "bors-kindergarten",
     "rust",
@@ -27,3 +33,25 @@ permissions-bools = [
     "dev-desktop",
     "sync-team-confirmation",
 ]
+
+# GitHub accounts that are allowed to stay in the GitHub organizations,
+# even if they may not be members of any team.
+# Note: Infra admins are automatically included from the infra-admins team.
+special-org-members = [
+    # Bots.
+    "bors",
+    "craterbot",
+    "rust-embedded-bot",
+    "rust-highfive",
+    "rust-lang-core-team-agenda-bot",
+    "rust-lang-glacier-bot",
+    "rust-lang-owner",
+    "rust-log-analyzer",
+    "rust-timer",
+    "rustbot",
+
+    # Ferrous systems employees who manage the github sponsors
+    # for rust-analyzer.
+    "MissHolmes",
+    "skade",
+]

src/data.rs

@@ -217,6 +217,26 @@ impl Data {
             .flatten()
             .any(|github_team| github_team.name == team_name && github_team.org == org)
     }
+
+    pub(crate) fn get_sync_team_config(&self) -> anyhow::Result<sync_team::Config> {
+        let mut special_org_members = self.config.special_org_members().clone();
+
+        // Add infra admins from the infra-admins team
+        let infra_admins_team = self
+            .team("infra-admins")
+            .context("cannot find infra-admins team")?;
+        let infra_admins_usernames = infra_admins_team
+            .raw_people()
+            .members
+            .iter()
+            .map(|m| m.github.clone());
+        special_org_members.extend(infra_admins_usernames);
+
+        Ok(sync_team::Config {
+            special_org_members,
+            independent_github_orgs: self.config.independent_github_orgs().clone(),
+        })
+    }
 }
 
 fn load_file<T: DeserializeOwned>(path: &Path) -> Result<T, Error> {

src/main.rs

@@ -590,5 +590,11 @@ fn perform_sync(opts: SyncOpts, data: Data) -> anyhow::Result<()> {
     let subcmd = opts.command.unwrap_or(SyncCommand::DryRun);
     let only_print_plan = matches!(subcmd, SyncCommand::PrintPlan);
     let dry_run = only_print_plan || matches!(subcmd, SyncCommand::DryRun);
-    run_sync_team(team_api, &services, dry_run, only_print_plan)
+    run_sync_team(
+        team_api,
+        &services,
+        dry_run,
+        only_print_plan,
+        data.get_sync_team_config()?,
+    )
 }

src/schema.rs

@@ -3,15 +3,18 @@ pub(crate) use crate::permissions::Permissions;
 use anyhow::{bail, format_err, Error};
 use serde::de::{Deserialize, Deserializer};
 use serde_untagged::UntaggedEnumVisitor;
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeSet, HashMap, HashSet};
 
 #[derive(serde_derive::Deserialize, Debug)]
 #[serde(deny_unknown_fields, rename_all = "kebab-case")]
 pub(crate) struct Config {
     allowed_mailing_lists_domains: HashSet<String>,
     allowed_github_orgs: HashSet<String>,
+    independent_github_orgs: BTreeSet<String>,
     permissions_bors_repos: HashSet<String>,
     permissions_bools: HashSet<String>,
+    // Use a BTreeSet for consistent ordering in tests
+    special_org_members: BTreeSet<String>,
 }
 
 impl Config {
@@ -30,6 +33,14 @@ impl Config {
     pub(crate) fn permissions_bools(&self) -> &HashSet<String> {
         &self.permissions_bools
     }
+
+    pub(crate) fn independent_github_orgs(&self) -> &BTreeSet<String> {
+        &self.independent_github_orgs
+    }
+
+    pub(crate) fn special_org_members(&self) -> &BTreeSet<String> {
+        &self.special_org_members
+    }
 }
 
 // This is an enum to allow two kinds of values for the email field:

sync-team/src/github/mod.rs

@@ -3,6 +3,7 @@ mod api;
 mod tests;
 
 use self::api::{BranchProtectionOp, TeamPrivacy, TeamRole};
+use crate::Config;
 use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings};
 use log::debug;
 use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot};
@@ -18,8 +19,9 @@ pub(crate) fn create_diff(
     github: Box<dyn GithubRead>,
     teams: Vec<rust_team_data::v1::Team>,
     repos: Vec<rust_team_data::v1::Repo>,
+    config: Config,
 ) -> anyhow::Result<Diff> {
-    let github = SyncGitHub::new(github, teams, repos)?;
+    let github = SyncGitHub::new(github, teams, repos, config)?;
     github.diff_all()
 }
 
@@ -29,6 +31,7 @@ struct SyncGitHub {
     github: Box<dyn GithubRead>,
     teams: Vec<rust_team_data::v1::Team>,
     repos: Vec<rust_team_data::v1::Repo>,
+    config: Config,
     usernames_cache: HashMap<u64, String>,
     org_owners: HashMap<OrgName, HashSet<u64>>,
     org_members: HashMap<OrgName, HashMap<u64, String>>,
@@ -39,6 +42,7 @@ impl SyncGitHub {
         github: Box<dyn GithubRead>,
         teams: Vec<rust_team_data::v1::Team>,
         repos: Vec<rust_team_data::v1::Repo>,
+        config: Config,
     ) -> anyhow::Result<Self> {
         debug!("caching mapping between user ids and usernames");
         let users = teams
@@ -72,6 +76,7 @@ impl SyncGitHub {
             github,
             teams,
             repos,
+            config,
             usernames_cache,
             org_owners,
             org_members,
@@ -90,7 +95,7 @@ impl SyncGitHub {
         })
     }
 
-    // Collect all org members from the respective teams
+    /// Collect all org members from the respective teams
     fn get_org_members_from_teams(&self) -> HashMap<OrgName, HashSet<u64>> {
         let mut org_team_members: HashMap<OrgName, HashSet<u64>> = HashMap::new();
 
@@ -107,22 +112,24 @@ impl SyncGitHub {
         org_team_members
     }
 
-    // Diff organization memberships between TOML teams and GitHub
+    /// Diff organization memberships between TOML teams and GitHub
     fn diff_org_memberships(&self) -> anyhow::Result<Vec<OrgMembershipDiff>> {
         let toml_org_team_members = self.get_org_members_from_teams();
 
         let mut org_diffs: BTreeMap<String, OrgMembershipDiff> = BTreeMap::new();
 
         for (org, toml_members) in toml_org_team_members {
+            // Skip independent organizations - they manage their own members
+            if self.config.independent_github_orgs.contains(&org) {
+                debug!("Skipping member sync for independent organization: {}", org);
+                continue;
+            }
+
             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);
-            }
+            let members_to_remove = self.members_to_remove(toml_members, gh_org_members);
 
             // The rest are members that should be removed
             if !members_to_remove.is_empty() {
@@ -142,6 +149,34 @@ impl SyncGitHub {
         Ok(org_diffs.into_values().collect())
     }
 
+    /// Return GitHub members that should be removed from the organization.
+    fn members_to_remove(
+        &self,
+        toml_members: HashSet<u64>,
+        gh_org_members: &HashMap<u64, String>,
+    ) -> HashMap<u64, String> {
+        // Initialize `members_to_remove` to all GitHub members in the org.
+        // Next, we'll delete members from `members_to_remove` that don't respect certain criteria.
+        let mut members_to_remove = gh_org_members.clone();
+
+        // People who belong to a team should stay in the org.
+        for member in toml_members {
+            members_to_remove.remove(&member);
+        }
+
+        // Members that are explicitly allowed in the `config.toml` file should stay in the org.
+        for allowed_member in &self.config.special_org_members {
+            if let Some(member_to_retain) = members_to_remove
+                .iter()
+                .find(|(_, username)| username == &allowed_member)
+                .map(|(id, _)| *id)
+            {
+                members_to_remove.remove(&member_to_retain);
+            }
+        }
+        members_to_remove
+    }
+
     fn diff_teams(&self) -> anyhow::Result<Vec<TeamDiff>> {
         let mut diffs = Vec::new();
         let mut unseen_github_teams = HashMap::new();
@@ -755,7 +790,7 @@ struct OrgMembershipDiff {
 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)?;
+            sync.remove_gh_member_from_org(&self.org, member)?;
         }
 
         Ok(())

sync-team/src/github/tests/mod.rs

@@ -100,10 +100,16 @@ fn team_dont_add_member_if_invitation_is_pending() {
 #[test]
 fn remove_org_members() {
     let mut model = DataModel::default();
+    let rust_lang_org = "rust-lang";
     let user = model.create_user("sakura");
-    model.create_team(TeamData::new("team-1").gh_team("rust-lang", "members-gh", &[user]));
+    model.create_team(TeamData::new("team-1").gh_team(rust_lang_org, "members-gh", &[user]));
     let mut gh = model.gh_model();
-    gh.add_member("rust-lang", "martin");
+    gh.add_member(rust_lang_org, "martin");
+
+    // Add a bot that shouldn't be removed from the org.
+    let bot = "my-bot";
+    gh.add_member(rust_lang_org, bot);
+    model.add_allowed_org_member(bot);
 
     let gh_org_diff = model.diff_org_membership(gh);
 
@@ -888,3 +894,26 @@ fn repo_remove_branch_protection() {
     ]
     "#);
 }
+
+#[test]
+fn independent_orgs_are_not_synced() {
+    let mut model = DataModel::default();
+    let user = model.create_user("sakura");
+
+    let independent_org = "independent-org";
+
+    // Create a team, so that membership is synced.
+    model.create_team(TeamData::new("team").gh_team(independent_org, "team-gh", &[user]));
+
+    let mut gh = model.gh_model();
+
+    // Add a member who is not part of any team.
+    gh.add_member(independent_org, "independent-user-1");
+
+    model.add_independent_github_org(independent_org);
+
+    let gh_org_diff = model.diff_org_membership(gh);
+
+    // No members should be removed for independent organizations
+    insta::assert_debug_snapshot!(gh_org_diff, @"[]");
+}

sync-team/src/github/tests/test_utils.rs

@@ -1,11 +1,12 @@
 use std::collections::{BTreeSet, HashMap, HashSet};
 
 use derive_builder::Builder;
-use rust_team_data::v1;
 use rust_team_data::v1::{
-    Bot, BranchProtectionMode, GitHubTeam, MergeBot, Person, RepoPermission, TeamGitHub, TeamKind,
+    self, Bot, BranchProtectionMode, GitHubTeam, MergeBot, Person, RepoPermission, TeamGitHub,
+    TeamKind,
 };
 
+use crate::Config;
 use crate::github::api::{
     BranchProtection, GithubRead, Repo, RepoTeam, RepoUser, Team, TeamMember, TeamPrivacy, TeamRole,
 };
@@ -28,6 +29,7 @@ pub struct DataModel {
     people: Vec<Person>,
     teams: Vec<TeamData>,
     repos: Vec<RepoData>,
+    config: Config,
 }
 
 impl DataModel {
@@ -65,6 +67,14 @@ impl DataModel {
             .expect("Repo not found")
     }
 
+    pub fn add_allowed_org_member(&mut self, member: &str) {
+        self.config.special_org_members.insert(member.to_string());
+    }
+
+    pub fn add_independent_github_org(&mut self, org: &str) {
+        self.config.independent_github_orgs.insert(org.to_string());
+    }
+
     /// Creates a GitHub model from the current team data mock.
     /// Note that all users should have been created before calling this method, so that
     /// GitHub knows about the users' existence.
@@ -196,8 +206,9 @@ impl DataModel {
     fn create_sync(&self, github: GithubMock) -> SyncGitHub {
         let teams = self.teams.iter().cloned().map(|t| t.into()).collect();
         let repos = self.repos.iter().cloned().map(|r| r.into()).collect();
+        let config = self.config.clone();
 
-        SyncGitHub::new(Box::new(github), teams, repos).expect("Cannot create SyncGitHub")
+        SyncGitHub::new(Box::new(github), teams, repos, config).expect("Cannot create SyncGitHub")
     }
 }
 
@@ -435,8 +446,8 @@ impl GithubMock {
         let user_id = self.users.len() as UserId;
         self.users.insert(user_id, username.to_string());
         self.orgs
-            .get_mut(org)
-            .unwrap()
+            .entry(org.to_string())
+            .or_default()
             .members
             .insert((user_id, username.to_string()));
     }

sync-team/src/lib.rs

@@ -4,6 +4,8 @@ pub mod team_api;
 mod utils;
 mod zulip;
 
+use std::collections::BTreeSet;
+
 use crate::github::{GitHubApiRead, GitHubWrite, HttpClient, create_diff};
 use crate::team_api::TeamApi;
 use crate::zulip::SyncZulip;
@@ -13,11 +15,18 @@ use secrecy::SecretString;
 
 const USER_AGENT: &str = "rust-lang teams sync (https://github.com/rust-lang/sync-team)";
 
+#[derive(Debug, Clone, Default)]
+pub struct Config {
+    pub special_org_members: BTreeSet<String>,
+    pub independent_github_orgs: BTreeSet<String>,
+}
+
 pub fn run_sync_team(
     team_api: TeamApi,
     services: &[String],
     dry_run: bool,
     only_print_plan: bool,
+    config: Config,
 ) -> anyhow::Result<()> {
     if dry_run {
         warn!("sync-team is running in dry mode, no changes will be applied.");
@@ -31,7 +40,7 @@ pub fn run_sync_team(
                 let gh_read = Box::new(GitHubApiRead::from_client(client.clone())?);
                 let teams = team_api.get_teams()?;
                 let repos = team_api.get_repos()?;
-                let diff = create_diff(gh_read, teams, repos)?;
+                let diff = create_diff(gh_read, teams, repos, config.clone())?;
                 if !diff.is_empty() {
                     info!("{diff}");
                 }

tests/static-api/_expected/v1/people.json

@@ -1,5 +1,10 @@
 {
   "people": {
+    "test-admin": {
+      "name": "Test Admin",
+      "email": "[email protected]",
+      "github_id": 7
+    },
     "user-0": {
       "name": "Zeroth user",
       "email": "[email protected]",