Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functionality to sync org members on GitHub #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/github/api/read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ pub(crate) trait GithubRead {
/// Get the owners of an org
fn org_owners(&self, org: &str) -> anyhow::Result<HashSet<u64>>;

/// Get the members of an org
fn org_members(&self, org: &str) -> anyhow::Result<HashSet<u64>>;

/// Get the app installations of an org
fn org_app_installations(&self, org: &str) -> anyhow::Result<Vec<OrgAppInstallation>>;

Expand Down Expand Up @@ -120,6 +123,23 @@ impl GithubRead for GitHubApiRead {
Ok(owners)
}

fn org_members(&self, org: &str) -> anyhow::Result<HashSet<u64>> {
#[derive(serde::Deserialize, Eq, PartialEq, Hash)]
struct User {
id: u64,
}
let mut members = HashSet::new();
self.client.rest_paginated(
&Method::GET,
format!("orgs/{org}/members"),
|resp: Vec<User>| {
members.extend(resp.into_iter().map(|u| u.id));
Ok(())
},
)?;
Ok(members)
}

fn org_app_installations(&self, org: &str) -> anyhow::Result<Vec<OrgAppInstallation>> {
#[derive(serde::Deserialize, Debug)]
struct InstallationPage {
Expand Down
12 changes: 12 additions & 0 deletions src/github/api/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,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 = &format!("orgs/{org}/members/{user}");
let resp = self.client.req(method.clone(), url)?.send()?;
allow_not_found(resp, method, url)?;
}
Ok(())
}

/// Remove a collaborator from a repo
pub(crate) fn remove_collaborator_from_repo(
&self,
Expand Down
108 changes: 108 additions & 0 deletions src/github/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ struct SyncGitHub {
repos: Vec<rust_team_data::v1::Repo>,
usernames_cache: HashMap<u64, String>,
org_owners: HashMap<OrgName, HashSet<u64>>,
org_members: HashMap<OrgName, HashSet<u64>>,
org_apps: HashMap<OrgName, Vec<OrgAppInstallation>>,
}

Expand Down Expand Up @@ -103,10 +104,12 @@ impl SyncGitHub {
.collect::<HashSet<_>>();

let mut org_owners = HashMap::new();
let mut org_members = HashMap::new();
let mut org_apps = 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)?);

let mut installations: Vec<OrgAppInstallation> = vec![];

Expand Down Expand Up @@ -134,17 +137,21 @@ impl SyncGitHub {
repos,
usernames_cache,
org_owners,
org_members,
org_apps,
})
}

pub(crate) fn diff_all(&self) -> anyhow::Result<Diff> {
let team_diffs = self.diff_teams()?;
let repo_diffs = self.diff_repos()?;
let org_team_members = self.map_teams_to_org()?;
let toml_github_diffs = self.diff_teams_gh_org(org_team_members)?;

Ok(Diff {
team_diffs,
repo_diffs,
toml_github_diffs,
})
}

Expand Down Expand Up @@ -195,6 +202,55 @@ impl SyncGitHub {
Ok(diffs)
}

// collect all org and respective teams members in a HashMap
fn map_teams_to_org(&self) -> anyhow::Result<HashMap<String, HashSet<u64>>> {
let mut org_team_members: HashMap<String, HashSet<u64>> = HashMap::new();

for team in &self.teams {
let mut team_org;

if let Some(gh) = &team.github {
for toml_gh_team in &gh.teams {
team_org = toml_gh_team.org.clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
team_org = toml_gh_team.org.clone();
let team_org = toml_gh_team.org.clone();

No need to have the variable in the external scope now :)

let toml_team_mems_gh_id: HashSet<u64> =
toml_gh_team.members.iter().copied().collect();

org_team_members
.entry(team_org)
.or_default()
.extend(toml_team_mems_gh_id);
}
}
}
Ok(org_team_members)
}

// create diff against github org members against toml team members
fn diff_teams_gh_org(
&self,
org_team_members: HashMap<String, HashSet<u64>>,
) -> anyhow::Result<OrgMembershipDiff> {
let mut org_with_members_to_be_removed: HashMap<String, HashSet<String>> = HashMap::new();

for (gh_org, toml_members_across_teams) in org_team_members.into_iter() {
let gh_org_members = self.org_members.get(&gh_org).unwrap();
Kobzol marked this conversation as resolved.
Show resolved Hide resolved

let members_to_be_removed = (&toml_members_across_teams - gh_org_members)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the other way around, we want to remove users (from GitHub) that are currently on GitHub, but are not in the TOML teams.

Suggested change
let members_to_be_removed = (&toml_members_across_teams - gh_org_members)
let members_to_be_removed = (gh_org_members - &toml_members_across_teams)

.into_iter()
.map(|user| self.usernames_cache[&user].clone())
.collect::<HashSet<String>>();

org_with_members_to_be_removed
.entry(gh_org)
.or_default()
.extend(members_to_be_removed);
}

Ok(OrgMembershipDiff::Delete(DeleteOrgMembershipDiff {
org_with_members: org_with_members_to_be_removed,
}))
}

fn diff_team(&self, github_team: &rust_team_data::v1::GitHubTeam) -> anyhow::Result<TeamDiff> {
// Ensure the team exists and is consistent
let team = match self.github.team(&github_team.org, &github_team.name)? {
Expand Down Expand Up @@ -667,6 +723,7 @@ const BOTS_TEAMS: &[&str] = &["bors", "highfive", "rfcbot", "bots"];
pub(crate) struct Diff {
team_diffs: Vec<TeamDiff>,
repo_diffs: Vec<RepoDiff>,
toml_github_diffs: OrgMembershipDiff,
}

impl Diff {
Expand All @@ -679,6 +736,8 @@ impl Diff {
repo_diff.apply(sync)?;
}

self.toml_github_diffs.apply(sync)?;

Ok(())
}
}
Expand Down Expand Up @@ -720,6 +779,55 @@ impl std::fmt::Display for RepoDiff {
}
}

#[derive(Debug)]

enum OrgMembershipDiff {
Kobzol marked this conversation as resolved.
Show resolved Hide resolved
Delete(DeleteOrgMembershipDiff),
}

impl OrgMembershipDiff {
fn apply(self, sync: &GitHubWrite) -> anyhow::Result<()> {
match self {
OrgMembershipDiff::Delete(d) => d.apply(sync)?,
}

Ok(())
}
}

impl std::fmt::Display for OrgMembershipDiff {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
OrgMembershipDiff::Delete(d) => write!(f, "{d}"),
}
}
}

#[derive(Debug)]

struct DeleteOrgMembershipDiff {
org_with_members: HashMap<String, HashSet<String>>,
}

impl DeleteOrgMembershipDiff {
fn apply(self, sync: &GitHubWrite) -> anyhow::Result<()> {
for (gh_org, members) in self.org_with_members.iter() {
for member in members {
sync.remove_gh_member_from_org(gh_org, member)?;
}
}

Ok(())
}
}

impl std::fmt::Display for DeleteOrgMembershipDiff {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
writeln!(f, "❌ Deleting members '{:?}'", self.org_with_members)?;
Ok(())
}
}

struct CreateRepoDiff {
org: String,
name: String,
Expand Down
27 changes: 27 additions & 0 deletions src/github/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,33 @@ fn team_dont_add_member_if_invitation_is_pending() {
"###);
}

#[test]
fn org_member_not_sync() {
let mut model = DataModel::default();
let user = model.create_user("sakura");
let user2 = model.create_user("hitori");
model.create_team(TeamData::new("team-1").gh_team("members-gh", &[user, user2]));
let gh = model.gh_model();

model
.get_team("team-1")
.remove_gh_member("members-gh", user);

let gh_org_diff = model.diff_toml_gh_org_teams(gh);

insta::assert_debug_snapshot!(gh_org_diff, @r###"
Delete(
DeleteOrgMembershipDiff {
org_with_members: {
"rust-lang": {
"hitori",
},
},
},
)
"###);
}

#[test]
fn team_remove_member() {
let mut model = DataModel::default();
Expand Down
34 changes: 32 additions & 2 deletions src/github/tests/test_utils.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use std::collections::{HashMap, HashSet};

use derive_builder::Builder;
use rust_team_data::v1::{GitHubTeam, Person, TeamGitHub, TeamKind};
use rust_team_data::v1::{GitHubTeam, Person, Team as TomlTeam, TeamGitHub, TeamKind};

use crate::github::api::{
BranchProtection, GithubRead, OrgAppInstallation, Repo, RepoAppInstallation, RepoTeam,
RepoUser, Team, TeamMember, TeamPrivacy, TeamRole,
};
use crate::github::{api, SyncGitHub, TeamDiff};
use crate::github::{api, OrgMembershipDiff, SyncGitHub, TeamDiff};

const DEFAULT_ORG: &str = "rust-lang";

Expand Down Expand Up @@ -92,12 +92,29 @@ impl DataModel {
GithubMock {
users,
owners: Default::default(),
members: Default::default(),
teams,
team_memberships,
team_invitations: Default::default(),
}
}

pub fn diff_toml_gh_org_teams(
&self,
github: GithubMock,
// members: HashSet<u64>,
) -> OrgMembershipDiff {
let teams: Vec<TomlTeam> = self.teams.iter().map(|r| r.to_data()).collect();
let repos = vec![];
let read = Box::new(github);
let sync = SyncGitHub::new(read, teams, repos).expect("Cannot create SyncGitHub");

let org_team_members = sync.map_teams_to_org().unwrap();

sync.diff_teams_gh_org(org_team_members)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test won't work yet, because our test API is not finished yet and this uses the unfinished parts. We need to properly track GH users and GH org members in GithubMock, which doesn't happen currently :( I will work on it and send a PR so that you can write the test and check it locally

.expect("Cannot diff toml teams")
}

pub fn diff_teams(&self, github: GithubMock) -> Vec<TeamDiff> {
let teams = self.teams.iter().map(|r| r.to_data()).collect();
let repos = vec![];
Expand Down Expand Up @@ -184,6 +201,9 @@ pub struct GithubMock {
// org name -> user ID
owners: HashMap<String, Vec<UserId>>,
teams: Vec<Team>,

// org name -> user ID (members)
members: HashMap<String, Vec<UserId>>,
// Team name -> members
team_memberships: HashMap<String, HashMap<UserId, TeamMember>>,
// Team name -> list of invited users
Expand Down Expand Up @@ -219,6 +239,16 @@ impl GithubRead for GithubMock {
.collect())
}

fn org_members(&self, org: &str) -> anyhow::Result<HashSet<u64>> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of it, I took inspiration from org_owners

Ok(self
.members
.get(org)
.cloned()
.unwrap_or_default()
.into_iter()
.collect())
}

fn org_app_installations(&self, _org: &str) -> anyhow::Result<Vec<OrgAppInstallation>> {
Ok(vec![])
}
Expand Down