Add functionality to sync org members on GitHub #91

me-diru · 2024-10-31T05:13:06Z

Fixes #84

Added GitHub api functionality to list all org members and remove members from a GitHub org
Created a new Diff called OrgMembershipDiff to deal with difference on GitHub and toml members in teams.

me-diru · 2024-10-31T05:16:57Z

Right now, the PR is incomplete, and I wanted to check in with you, @marcoieni, to see if I was going in the right direction.

I made a new Diff against teams as suggested. Many thanks for the notes. They helped me understand the task well!

I am not sure about the tests checking the debugging output, but I also took a stab at it. I would really appreciate your input and feedback :D

me-diru · 2024-10-31T05:18:26Z

src/github/tests/test_utils.rs

@@ -219,6 +233,16 @@ impl GithubRead for GithubMock {
            .collect())
    }

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


most of it, I took inspiration from org_owners

src/github/mod.rs

marcoieni · 2024-10-31T14:36:29Z

src/github/mod.rs

+                .expect("Team Not Found");
+            team_org = github_team.org.clone();
+        } else {
+            team_org = "rust_lang".to_string();


why this value?
Should we report an error if the team doesn't have any associated org?
Also the rust org is called "rust-lang"

marcoieni · 2024-10-31T14:42:36Z

src/github/mod.rs

+
+        for toml_team_member in toml_team_members {
+            if gh_org_members.contains(&toml_team_member.github_id.clone()) {
+                members_to_be_removed.insert(toml_team_member.github.clone());


doesn't this remove all members from the org?

marcoieni · 2024-10-31T14:45:31Z

The general direction looks great 👍👍👍
Imo you are very close 😃

Kobzol · 2024-10-31T18:12:13Z

src/github/mod.rs

@@ -189,6 +196,48 @@ impl SyncGitHub {
        Ok(diffs)
    }

+    fn diff_toml_teams(&self) -> anyhow::Result<Vec<OrgMembershipDiff>> {


I think that it would be simpler and more robust implementation would be to do this instead:

Go through all teams and gather a set of all team members for each org that we find (so essentially a HashMap<Org, HashSet<u64>>)

Go through each org, and do a set difference between its members and the team members found in 1). Users that come out of this operation (those that are not in the team) should be stored in the OrgMembershipDiff

marcoieni · 2024-11-15T13:48:13Z

Hey Rohit, do you still plan to work on this?
No pressure, you already did great progress.
I just wanted to check to know if we should reassign the issue 👍

me-diru · 2024-11-15T21:08:05Z

Hey @marcoieni !

Sorry for disappearing. I will work on it over the weekend and get it past the finish line :D Appreciate your patience !

marcoieni · 2024-11-15T21:16:21Z

No worries, thanks 😁

me-diru · 2024-11-24T07:08:47Z

src/github/tests/mod.rs

+    model.create_team(TeamData::new("team-1").gh_team("members-gh", &[user, user2]));
+    let gh = model.gh_model();
+
+    // let gh_members = gh.org_members("rust-lang").unwrap();


not able to get the gh_members, it returns an empty hashset

Kobzol

Thanks for making the changes! Left a bunch of comments, mostly refactoring and some logic changes.

Kobzol · 2024-11-25T08:46:31Z

src/github/mod.rs

+                    .expect("Team Not Found");
+                team_org = github_team.org.clone();
+            } else {
+                return Err(anyhow!(


This should not be an error, some Rust teams simply don't have any GitHub team attached.

Kobzol · 2024-11-25T08:49:05Z

src/github/mod.rs

+            // get the team github org through the corresponding github team
+            if let Some(gh) = &team.github {
+                let github_teams = &gh.teams;
+                let github_team: &GitHubTeam = github_teams


We need to go through all teams, not just ones that have the same name. You can see the structure here, take a look at the [[github]] section. You can see how you can iterate through every GitHub tam in diff_teams.

Thanks for the feedback. I thought we have to go through all the toml teams first as per the notes by @marcoieni and then find the corresponding GitHubTeam and it's organisation through that.

I am not sure how to map the GitHub org to a respective toml team in a different way.

We could go through all the teams in TeamGitHub but if there are no corresponding toml team with the same name, I don't think it helps to go through all the teams.

I think that this was an error in the original description. Note that TeamGithub has nothing to do with the GitHub API. It's a representation of a GH team described in a TOML file. You can find that out easily from the fact that the corresponding data structure (TeamGitHub) is from the rust_team_data::v1 module. Everything that comes from this module is derived from the TOML files.

The logic should be something like this:

Go through all TOML teams (v1::Team)

For each TOML team, go through all its TOML GitHub teams (GitHubTeam)

For each TOML GitHub team, add its members to the corresponding organization in org_team_members

src/github/mod.rs

Kobzol · 2024-11-25T08:56:09Z

src/github/mod.rs

+        for (gh_org, toml_members_across_teams) in org_team_members.into_iter() {
+            let gh_org_members = self.org_members.get(&gh_org).unwrap();
+
+            let mut members_to_be_removed = HashSet::new();


What about this?

let members_to_be_removed = (gh_org_members - &toml_members_across_teams) .into_iter() .map(|user| self.usernames_cache[&user].clone()) .collect::<HashSet<String>>();

src/github/mod.rs

Kobzol · 2024-11-25T09:02:52Z

src/github/tests/mod.rs

+        .get_team("team-1")
+        .remove_gh_member("members-gh", user);
+
+    let gh_org_diff = model.diff_teams_gh_org(gh, gh_members);


This function should only receive gh, not any additional parameters. The logic here is not fully correct, as you're passing GH members, but the function should diff team (TOML) members. The list of member should be taken from the GithubMock.

Signed-off-by: Rohit Dandamudi <[email protected]>

Kobzol · 2024-12-02T12:26:15Z

src/github/mod.rs

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


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 :)

Kobzol · 2024-12-02T12:28:07Z

src/github/mod.rs

+        for (gh_org, toml_members_across_teams) in org_team_members.into_iter() {
+            let gh_org_members = self.org_members.get(&gh_org).unwrap();
+
+            let members_to_be_removed = (&toml_members_across_teams - gh_org_members)


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)

Kobzol · 2024-12-02T12:44:25Z

src/github/tests/test_utils.rs

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


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

me-diru commented Oct 31, 2024

View reviewed changes

src/github/mod.rs Show resolved Hide resolved

marcoieni mentioned this pull request Oct 31, 2024

Remove users without a team from GitHub #88

Closed

marcoieni reviewed Oct 31, 2024

View reviewed changes

Kobzol reviewed Oct 31, 2024

View reviewed changes

me-diru force-pushed the sync-org-mems branch 4 times, most recently from b657699 to 470218d Compare November 24, 2024 07:07

me-diru commented Nov 24, 2024

View reviewed changes

me-diru force-pushed the sync-org-mems branch from 470218d to 17878b6 Compare November 24, 2024 07:11

me-diru marked this pull request as ready for review November 24, 2024 07:12

me-diru changed the title ~~[WIP] Add functionality to sync org members on GitHub~~ Add functionality to sync org members on GitHub Nov 24, 2024

me-diru requested review from marcoieni and Kobzol November 24, 2024 07:15

Kobzol reviewed Nov 25, 2024

View reviewed changes

Add functionality to sync org members on GitHub

f2d7303

Signed-off-by: Rohit Dandamudi <[email protected]>

me-diru force-pushed the sync-org-mems branch from 17878b6 to f2d7303 Compare December 2, 2024 09:34

me-diru requested a review from Kobzol December 2, 2024 09:39

Kobzol reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to sync org members on GitHub #91

Add functionality to sync org members on GitHub #91

me-diru commented Oct 31, 2024

me-diru commented Oct 31, 2024

me-diru Oct 31, 2024

marcoieni Oct 31, 2024

marcoieni Oct 31, 2024

marcoieni commented Oct 31, 2024 •

edited

Loading

Kobzol Oct 31, 2024 •

edited

Loading

marcoieni commented Nov 15, 2024 •

edited

Loading

me-diru commented Nov 15, 2024

marcoieni commented Nov 15, 2024

me-diru Nov 24, 2024

Kobzol left a comment •

edited

Loading

Kobzol Nov 25, 2024

Kobzol Nov 25, 2024

me-diru Dec 2, 2024 •

edited

Loading

Kobzol Dec 2, 2024 •

edited

Loading

Kobzol Nov 25, 2024

Kobzol Nov 25, 2024

Kobzol Dec 2, 2024

Kobzol Dec 2, 2024

Kobzol Dec 2, 2024

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

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


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

		sync.diff_teams_gh_org(org_team_members)

Add functionality to sync org members on GitHub #91

Are you sure you want to change the base?

Add functionality to sync org members on GitHub #91

Conversation

me-diru commented Oct 31, 2024

me-diru commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoieni commented Oct 31, 2024 • edited Loading

Kobzol Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

marcoieni commented Nov 15, 2024 • edited Loading

me-diru commented Nov 15, 2024

marcoieni commented Nov 15, 2024

Choose a reason for hiding this comment

Kobzol left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

me-diru Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Kobzol Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoieni commented Oct 31, 2024 •

edited

Loading

Kobzol Oct 31, 2024 •

edited

Loading

marcoieni commented Nov 15, 2024 •

edited

Loading

Kobzol left a comment •

edited

Loading

me-diru Dec 2, 2024 •

edited

Loading

Kobzol Dec 2, 2024 •

edited

Loading