-
Notifications
You must be signed in to change notification settings - Fork 17
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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>>, | ||||||
} | ||||||
|
||||||
|
@@ -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![]; | ||||||
|
||||||
|
@@ -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, | ||||||
}) | ||||||
} | ||||||
|
||||||
|
@@ -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(); | ||||||
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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
.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)? { | ||||||
|
@@ -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 { | ||||||
|
@@ -679,6 +736,8 @@ impl Diff { | |||||
repo_diff.apply(sync)?; | ||||||
} | ||||||
|
||||||
self.toml_github_diffs.apply(sync)?; | ||||||
|
||||||
Ok(()) | ||||||
} | ||||||
} | ||||||
|
@@ -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, | ||||||
|
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"; | ||
|
||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
.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![]; | ||
|
@@ -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 | ||
|
@@ -219,6 +239,16 @@ impl GithubRead for GithubMock { | |
.collect()) | ||
} | ||
|
||
fn org_members(&self, org: &str) -> anyhow::Result<HashSet<u64>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. most of it, I took inspiration from |
||
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![]) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have the variable in the external scope now :)