From fa3aed1fb700e0a60b7ebb745aca66bf5335cdba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 27 Dec 2020 21:17:26 +0000 Subject: [PATCH 01/15] add struct to track invited github ids --- src/cli.rs | 9 +++++ src/invites.rs | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/main.rs | 1 + 3 files changed, 104 insertions(+) create mode 100644 src/invites.rs diff --git a/src/cli.rs b/src/cli.rs index 5ea3421..cfdb152 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -80,13 +80,22 @@ pub struct ListTeamParams { #[derive(Debug)] pub enum ExitError { Io(std::io::Error), + InvalidGitHubID(std::num::ParseIntError), Serde(serde_json::error::Error), } + impl From for ExitError { fn from(e: std::io::Error) -> Self { Self::Io(e) } } + +impl From for ExitError { + fn from(e: std::num::ParseIntError) -> Self { + Self::InvalidGitHubID(e) + } +} + impl From for ExitError { fn from(e: serde_json::error::Error) -> Self { Self::Serde(e) diff --git a/src/invites.rs b/src/invites.rs new file mode 100644 index 0000000..a12a93b --- /dev/null +++ b/src/invites.rs @@ -0,0 +1,94 @@ +use crate::cli::ExitError; +use crate::maintainers::GitHubID; +use std::collections::HashSet; +use std::fs::File; +use std::io::{BufRead, BufReader, Write}; +use std::path::Path; + +#[cfg_attr(test, derive(Debug, PartialEq))] +struct Invites { + invited: HashSet, +} + +impl Invites { + #[cfg(test)] + pub fn new() -> Invites { + Invites { + invited: HashSet::new(), + } + } + + pub fn load(path: &Path) -> Result { + let file = File::open(path)?; + let lines = BufReader::new(file).lines(); + + let mut invited = HashSet::new(); + for line in lines { + invited.insert(GitHubID::new(line?.parse()?)); + } + + Ok(Invites { invited }) + } + + pub fn save(&self, path: &Path) -> Result<(), ExitError> { + let mut file = File::create(path)?; + + let string = self + .invited + .iter() + .map(|id| id.to_string()) + .collect::>() + .join("\n"); + + file.write_all(string.as_ref())?; + + Ok(()) + } + + pub fn invited(&self, id: &GitHubID) -> bool { + self.invited.contains(id) + } + + pub fn add_invite(&mut self, id: GitHubID) { + self.invited.insert(id); + } + + pub fn remove_invite(&mut self, id: &GitHubID) { + self.invited.remove(id); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_load_save() { + let mut invites = Invites::new(); + let tmpdir = tempfile::tempdir().unwrap(); + let tmpfile = tmpdir.path().join("invites.txt"); + + for n in (0..20).step_by(3) { + invites.add_invite(GitHubID::new(n)); + } + + invites.save(&tmpfile).unwrap(); + + let loaded_invites = Invites::load(&tmpfile).unwrap(); + + assert_eq!(invites, loaded_invites); + } + + #[test] + fn test_add_remove_invites() { + let mut invites = Invites::new(); + + assert!(!invites.invited(&GitHubID::new(0))); + + invites.add_invite(GitHubID::new(0)); + assert!(invites.invited(&GitHubID::new(0))); + + invites.remove_invite(GitHubID::new(0)); + assert!(!invites.invited(&GitHubID::new(0))); + } +} diff --git a/src/main.rs b/src/main.rs index da6551b..87c7ce7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,6 +23,7 @@ use std::path::{Path, PathBuf}; use structopt::StructOpt; mod cli; use cli::{ExecMode, ExitError, Options}; +mod invites; mod maintainers; use maintainers::MaintainerList; mod filemunge; From cc30e34bb1483bfc56f10f07cad781a326a9830f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 27 Dec 2020 22:01:01 +0000 Subject: [PATCH 02/15] track previously invited users --- src/cli.rs | 3 +++ src/invites.rs | 5 ++--- src/main.rs | 1 + src/nix.rs | 2 +- src/op_sync_team.rs | 42 ++++++++++++++++++++++++++++++++++-------- 5 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index cfdb152..8173f87 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -70,6 +70,9 @@ pub struct SyncTeamParams { #[structopt(long = "limit")] pub limit: Option, + + #[structopt(long = "invited-list")] + pub invited_list: Option, } #[derive(Debug, StructOpt)] diff --git a/src/invites.rs b/src/invites.rs index a12a93b..efcc517 100644 --- a/src/invites.rs +++ b/src/invites.rs @@ -6,12 +6,11 @@ use std::io::{BufRead, BufReader, Write}; use std::path::Path; #[cfg_attr(test, derive(Debug, PartialEq))] -struct Invites { +pub struct Invites { invited: HashSet, } impl Invites { - #[cfg(test)] pub fn new() -> Invites { Invites { invited: HashSet::new(), @@ -88,7 +87,7 @@ mod tests { invites.add_invite(GitHubID::new(0)); assert!(invites.invited(&GitHubID::new(0))); - invites.remove_invite(GitHubID::new(0)); + invites.remove_invite(&GitHubID::new(0)); assert!(!invites.invited(&GitHubID::new(0))); } } diff --git a/src/main.rs b/src/main.rs index 87c7ce7..840abc4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -179,6 +179,7 @@ fn execute_ops(logger: slog::Logger, inputs: Options) -> Result<(), ExitError> { team_info.team_id, team_info.dry_run, team_info.limit, + team_info.invited_list, ), ExecMode::ListTeams(team_info) => op_sync_team::list_teams(github, &team_info.organization), } diff --git a/src/nix.rs b/src/nix.rs index 8d70b53..9c3f021 100644 --- a/src/nix.rs +++ b/src/nix.rs @@ -50,7 +50,7 @@ where cmd.arg(val); } - let output = cmd.output().expect("F;ailed to start nix-instantiate!"); + let output = cmd.output().expect("Failed to start nix-instantiate!"); if !output.stderr.is_empty() { warn!(logger, "Stderr from nix-instantiate"; diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index dba3748..b3b5021 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -1,4 +1,5 @@ use crate::cli::ExitError; +use crate::invites::Invites; use crate::maintainers::{GitHubID, GitHubName, Handle, MaintainerList}; use futures::stream::Stream; use hubcaps::teams::{TeamMemberOptions, TeamMemberRole}; @@ -6,6 +7,7 @@ use hubcaps::Github; use prometheus::{Histogram, IntCounter, IntGauge}; use std::collections::HashMap; use std::convert::TryInto; +use std::path::PathBuf; use tokio::runtime::Runtime; lazy_static! { @@ -36,6 +38,7 @@ pub fn sync_team( team_id: u64, dry_run: bool, limit: Option, + invited_list: Option, ) -> Result<(), ExitError> { // initialize the counters :( GITHUB_CALLS.get(); @@ -143,6 +146,12 @@ pub fn sync_team( current_team_member_gauge.set(current_members.len().try_into().unwrap()); + let mut invites = if let Some(ref invited_list) = invited_list { + Invites::load(invited_list)? + } else { + Invites::new() + }; + debug!(logger, "Fetching existing invitations"); let pending_invites: Vec = rt .block_on( @@ -202,19 +211,24 @@ pub fn sync_team( } } match action { - TeamAction::Add(github_name, handle) => { + TeamAction::Add(github_name, github_id, handle) => { let logger = logger.new(o!( "nixpkgs-handle" => format!("{}", handle), "github-name" => format!("{}", github_name), )); - if pending_invites.contains(&github_name) { + if pending_invites.contains(&github_name) || invites.invited(&github_id) { noops.inc(); - debug!(logger, "User already has a pending invitation"); + debug!(logger, "User has already been invited previously and/or still has a pending invitation"); } else { additions.inc(); info!(logger, "Adding user to the team"); if do_it_live { + // keep track of the invitation locally so that we don't + // spam users that have already been invited and rejected + // the invitation + invites.add_invite(github_id.clone()); + // verify the ID and name still match let get_user = rt.block_on( github.users().get(&format!("{}", github_name)), @@ -268,13 +282,15 @@ pub fn sync_team( noops.inc(); trace!(logger, "Keeping user on the team"); } - TeamAction::Remove(github_name) => { + TeamAction::Remove(github_name, github_id) => { let logger = logger.new(o!( "github-name" => format!("{}", github_name), )); removals.inc(); info!(logger, "Removing user from the team"); if do_it_live { + invites.remove_invite(&github_id); + // verify the ID and name still match let get_user = rt .block_on( @@ -315,6 +331,10 @@ pub fn sync_team( } } + if let Some(ref invited_list) = invited_list { + invites.save(invited_list)?; + } + Ok(()) } @@ -345,8 +365,8 @@ impl TrackedReactor { #[derive(Debug, PartialEq)] enum TeamAction { - Add(GitHubName, Handle), - Remove(GitHubName), + Add(GitHubName, GitHubID, Handle), + Remove(GitHubName, GitHubID), Keep(Handle), } @@ -379,7 +399,10 @@ fn maintainer_team_diff( if teammembers.contains_key(&m.github_id?) { Some((m.github_id?, TeamAction::Keep(handle))) } else { - Some((m.github_id?, TeamAction::Add(m.github?, handle))) + Some(( + m.github_id?, + TeamAction::Add(m.github?, m.github_id?, handle), + )) } }) .collect(); @@ -388,7 +411,10 @@ fn maintainer_team_diff( // the diff list already has an entry for who should be in it // now create removals for who should no longer be present if !diff.contains_key(github_id) { - diff.insert(*github_id, TeamAction::Remove(github_name.clone())); + diff.insert( + *github_id, + TeamAction::Remove(github_name.clone(), *github_id), + ); } } From 3a1744bbffd0fa26e4a7ffffafd2eafab91d342e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 27 Dec 2020 22:07:18 +0000 Subject: [PATCH 03/15] fix tests --- src/op_sync_team.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index b3b5021..9ba1ad0 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -464,12 +464,16 @@ mod tests { vec![ ( GitHubID::new(1), - TeamAction::Remove(GitHubName::new("alice")) + TeamAction::Remove(GitHubName::new("alice"), GitHubID::new(1)) ), (GitHubID::new(2), TeamAction::Keep(Handle::new("bob"))), ( GitHubID::new(3), - TeamAction::Add(GitHubName::new("charlie"), Handle::new("charlie")) + TeamAction::Add( + GitHubName::new("charlie"), + GitHubID::new(3), + Handle::new("charlie") + ) ), ] .into_iter() From 200987a89f80789930148eda2ed998ef29ff4733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 27 Dec 2020 22:07:25 +0000 Subject: [PATCH 04/15] add docs to --invited-list --- src/cli.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cli.rs b/src/cli.rs index 8173f87..752c0dd 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -71,6 +71,9 @@ pub struct SyncTeamParams { #[structopt(long = "limit")] pub limit: Option, + /// File to track previously invited users. Setting this parameter + /// guarantees that users that have been previously invited and rejected + /// will not keep getting spammed. #[structopt(long = "invited-list")] pub invited_list: Option, } From 9624a6e796f0b76ad253583cd60bbc1ad0d530e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 27 Dec 2020 22:17:13 +0000 Subject: [PATCH 05/15] rename Invites to Invited --- src/{invites.rs => invited.rs} | 44 +++++++++++++++++----------------- src/main.rs | 2 +- src/op_sync_team.rs | 16 ++++++------- 3 files changed, 31 insertions(+), 31 deletions(-) rename src/{invites.rs => invited.rs} (55%) diff --git a/src/invites.rs b/src/invited.rs similarity index 55% rename from src/invites.rs rename to src/invited.rs index efcc517..f005173 100644 --- a/src/invites.rs +++ b/src/invited.rs @@ -6,18 +6,18 @@ use std::io::{BufRead, BufReader, Write}; use std::path::Path; #[cfg_attr(test, derive(Debug, PartialEq))] -pub struct Invites { +pub struct Invited { invited: HashSet, } -impl Invites { - pub fn new() -> Invites { - Invites { +impl Invited { + pub fn new() -> Invited { + Invited { invited: HashSet::new(), } } - pub fn load(path: &Path) -> Result { + pub fn load(path: &Path) -> Result { let file = File::open(path)?; let lines = BufReader::new(file).lines(); @@ -26,7 +26,7 @@ impl Invites { invited.insert(GitHubID::new(line?.parse()?)); } - Ok(Invites { invited }) + Ok(Invited { invited }) } pub fn save(&self, path: &Path) -> Result<(), ExitError> { @@ -44,15 +44,15 @@ impl Invites { Ok(()) } - pub fn invited(&self, id: &GitHubID) -> bool { + pub fn contains(&self, id: &GitHubID) -> bool { self.invited.contains(id) } - pub fn add_invite(&mut self, id: GitHubID) { + pub fn add(&mut self, id: GitHubID) { self.invited.insert(id); } - pub fn remove_invite(&mut self, id: &GitHubID) { + pub fn remove(&mut self, id: &GitHubID) { self.invited.remove(id); } } @@ -63,31 +63,31 @@ mod tests { #[test] fn test_load_save() { - let mut invites = Invites::new(); + let mut invited = Invited::new(); let tmpdir = tempfile::tempdir().unwrap(); - let tmpfile = tmpdir.path().join("invites.txt"); + let tmpfile = tmpdir.path().join("invited.txt"); for n in (0..20).step_by(3) { - invites.add_invite(GitHubID::new(n)); + invited.add(GitHubID::new(n)); } - invites.save(&tmpfile).unwrap(); + invited.save(&tmpfile).unwrap(); - let loaded_invites = Invites::load(&tmpfile).unwrap(); + let loaded_invited = Invited::load(&tmpfile).unwrap(); - assert_eq!(invites, loaded_invites); + assert_eq!(invited, loaded_invited); } #[test] - fn test_add_remove_invites() { - let mut invites = Invites::new(); + fn test_add_remove_invited() { + let mut invited = Invited::new(); - assert!(!invites.invited(&GitHubID::new(0))); + assert!(!invited.contains(&GitHubID::new(0))); - invites.add_invite(GitHubID::new(0)); - assert!(invites.invited(&GitHubID::new(0))); + invited.add(GitHubID::new(0)); + assert!(invited.contains(&GitHubID::new(0))); - invites.remove_invite(&GitHubID::new(0)); - assert!(!invites.invited(&GitHubID::new(0))); + invited.remove(&GitHubID::new(0)); + assert!(!invited.contains(&GitHubID::new(0))); } } diff --git a/src/main.rs b/src/main.rs index 840abc4..b99fbc4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,7 +23,7 @@ use std::path::{Path, PathBuf}; use structopt::StructOpt; mod cli; use cli::{ExecMode, ExitError, Options}; -mod invites; +mod invited; mod maintainers; use maintainers::MaintainerList; mod filemunge; diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index 9ba1ad0..b0c76dc 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -1,5 +1,5 @@ use crate::cli::ExitError; -use crate::invites::Invites; +use crate::invited::Invited; use crate::maintainers::{GitHubID, GitHubName, Handle, MaintainerList}; use futures::stream::Stream; use hubcaps::teams::{TeamMemberOptions, TeamMemberRole}; @@ -146,10 +146,10 @@ pub fn sync_team( current_team_member_gauge.set(current_members.len().try_into().unwrap()); - let mut invites = if let Some(ref invited_list) = invited_list { - Invites::load(invited_list)? + let mut invited = if let Some(ref invited_list) = invited_list { + Invited::load(invited_list)? } else { - Invites::new() + Invited::new() }; debug!(logger, "Fetching existing invitations"); @@ -216,7 +216,7 @@ pub fn sync_team( "nixpkgs-handle" => format!("{}", handle), "github-name" => format!("{}", github_name), )); - if pending_invites.contains(&github_name) || invites.invited(&github_id) { + if pending_invites.contains(&github_name) || invited.contains(&github_id) { noops.inc(); debug!(logger, "User has already been invited previously and/or still has a pending invitation"); } else { @@ -227,7 +227,7 @@ pub fn sync_team( // keep track of the invitation locally so that we don't // spam users that have already been invited and rejected // the invitation - invites.add_invite(github_id.clone()); + invited.add(github_id.clone()); // verify the ID and name still match let get_user = rt.block_on( @@ -289,7 +289,7 @@ pub fn sync_team( removals.inc(); info!(logger, "Removing user from the team"); if do_it_live { - invites.remove_invite(&github_id); + invited.remove(&github_id); // verify the ID and name still match let get_user = rt @@ -332,7 +332,7 @@ pub fn sync_team( } if let Some(ref invited_list) = invited_list { - invites.save(invited_list)?; + invited.save(invited_list)?; } Ok(()) From 70a57e2670ea64ce42c705c802413cfbd890e995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 27 Dec 2020 22:24:22 +0000 Subject: [PATCH 06/15] create invited file if it doesn't exist --- src/invited.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/invited.rs b/src/invited.rs index f005173..d201362 100644 --- a/src/invited.rs +++ b/src/invited.rs @@ -1,7 +1,7 @@ use crate::cli::ExitError; use crate::maintainers::GitHubID; use std::collections::HashSet; -use std::fs::File; +use std::fs::{File, OpenOptions}; use std::io::{BufRead, BufReader, Write}; use std::path::Path; @@ -18,7 +18,14 @@ impl Invited { } pub fn load(path: &Path) -> Result { - let file = File::open(path)?; + // we want to create the file if it doesn't exist even though we won't + // be writing to it, this just makes the API easier to use. + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .open(path)?; + let lines = BufReader::new(file).lines(); let mut invited = HashSet::new(); @@ -78,6 +85,16 @@ mod tests { assert_eq!(invited, loaded_invited); } + #[test] + fn test_load_creates_file_if_doesnt_exist() { + let tmpdir = tempfile::tempdir().unwrap(); + let tmpfile = tmpdir.path().join("invited.txt"); + + let invited = Invited::load(&tmpfile).unwrap(); + + assert_eq!(invited, Invited::new()); + } + #[test] fn test_add_remove_invited() { let mut invited = Invited::new(); From 25ce51c3c569023659eefe3b0a793824966a7fd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Sun, 27 Dec 2020 22:34:46 +0000 Subject: [PATCH 07/15] parse invited list option with from_os_str --- src/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cli.rs b/src/cli.rs index 752c0dd..6194d1b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -74,7 +74,7 @@ pub struct SyncTeamParams { /// File to track previously invited users. Setting this parameter /// guarantees that users that have been previously invited and rejected /// will not keep getting spammed. - #[structopt(long = "invited-list")] + #[structopt(long = "invited-list", parse(from_os_str))] pub invited_list: Option, } From a99fa5f305ac7893b09181bafbb89f1a34e5a1d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 11:04:10 +0000 Subject: [PATCH 08/15] only add/remove invited if successful --- src/op_sync_team.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index b0c76dc..532925b 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -224,11 +224,6 @@ pub fn sync_team( info!(logger, "Adding user to the team"); if do_it_live { - // keep track of the invitation locally so that we don't - // spam users that have already been invited and rejected - // the invitation - invited.add(github_id.clone()); - // verify the ID and name still match let get_user = rt.block_on( github.users().get(&format!("{}", github_name)), @@ -263,11 +258,15 @@ pub fn sync_team( ); match add_attempt { - Ok(_) => (), + Ok(_) => { + // keep track of the invitation locally so that we don't + // spam users that have already been invited and rejected + // the invitation + invited.add(github_id.clone()); + } Err(e) => { errors.inc(); - warn!(logger, "Failed to add a user to the team, not decrementing additions as it may have succeeded: {:#?}", e - ); + warn!(logger, "Failed to add a user to the team, not decrementing additions as it may have succeeded: {:#?}", e); } } } @@ -289,8 +288,6 @@ pub fn sync_team( removals.inc(); info!(logger, "Removing user from the team"); if do_it_live { - invited.remove(&github_id); - // verify the ID and name still match let get_user = rt .block_on( @@ -316,14 +313,19 @@ pub fn sync_team( } }); - if let Ok(Some(_user)) = get_user { - if let Err(e) = rt.block_on( + if let Ok(Some(_)) = get_user { + let remove_attempt = rt.block_on( team_actions.remove_user(&format!("{}", github_name)), &github_remove_user_histogram, &github_remove_user_failures, - ) { - errors.inc(); - warn!(logger, "Failed to remove a user from the team: {:#?}", e); + ); + + match remove_attempt { + Ok(_) => invited.remove(&github_id), + Err(e) => { + errors.inc(); + warn!(logger, "Failed to remove a user from the team: {:#?}", e); + } } } } From 0752d205952c6171e581b76f736fcd4e3c8e2cb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 11:07:24 +0000 Subject: [PATCH 09/15] distinguish between pending invite / rejected invite --- src/op_sync_team.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index 532925b..9330f54 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -216,9 +216,13 @@ pub fn sync_team( "nixpkgs-handle" => format!("{}", handle), "github-name" => format!("{}", github_name), )); - if pending_invites.contains(&github_name) || invited.contains(&github_id) { + + if pending_invites.contains(&github_name) { + noops.inc(); + debug!(logger, "User already has a pending invitation"); + } else if invited.contains(&github_id) { noops.inc(); - debug!(logger, "User has already been invited previously and/or still has a pending invitation"); + debug!(logger, "User was already invited previously (since there's no pending invitation we can assume the user rejected the invite)"); } else { additions.inc(); info!(logger, "Adding user to the team"); From b796257f764eae7523766219a24f71c249e19649 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 15:39:10 +0000 Subject: [PATCH 10/15] make invited list file mandatory --- src/cli.rs | 2 +- src/invited.rs | 1 + src/main.rs | 2 +- src/op_sync_team.rs | 12 +++--------- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 6194d1b..789d329 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -75,7 +75,7 @@ pub struct SyncTeamParams { /// guarantees that users that have been previously invited and rejected /// will not keep getting spammed. #[structopt(long = "invited-list", parse(from_os_str))] - pub invited_list: Option, + pub invited_list: PathBuf, } #[derive(Debug, StructOpt)] diff --git a/src/invited.rs b/src/invited.rs index d201362..cd92f92 100644 --- a/src/invited.rs +++ b/src/invited.rs @@ -11,6 +11,7 @@ pub struct Invited { } impl Invited { + #[cfg(test)] pub fn new() -> Invited { Invited { invited: HashSet::new(), diff --git a/src/main.rs b/src/main.rs index b99fbc4..1be17d0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -175,11 +175,11 @@ fn execute_ops(logger: slog::Logger, inputs: Options) -> Result<(), ExitError> { logger.new(o!("exec-mode" => "SyncTeam")), github, maintainers, + team_info.invited_list, &team_info.organization, team_info.team_id, team_info.dry_run, team_info.limit, - team_info.invited_list, ), ExecMode::ListTeams(team_info) => op_sync_team::list_teams(github, &team_info.organization), } diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index 9330f54..cf09b28 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -34,11 +34,11 @@ pub fn sync_team( logger: slog::Logger, github: Github, maintainers: MaintainerList, + invited_list: PathBuf, org: &str, team_id: u64, dry_run: bool, limit: Option, - invited_list: Option, ) -> Result<(), ExitError> { // initialize the counters :( GITHUB_CALLS.get(); @@ -146,11 +146,7 @@ pub fn sync_team( current_team_member_gauge.set(current_members.len().try_into().unwrap()); - let mut invited = if let Some(ref invited_list) = invited_list { - Invited::load(invited_list)? - } else { - Invited::new() - }; + let mut invited = Invited::load(&invited_list)?; debug!(logger, "Fetching existing invitations"); let pending_invites: Vec = rt @@ -337,9 +333,7 @@ pub fn sync_team( } } - if let Some(ref invited_list) = invited_list { - invited.save(invited_list)?; - } + invited.save(&invited_list)?; Ok(()) } From c27345530a6bdbacc01825b26dae1bb303d344e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 15:50:51 +0000 Subject: [PATCH 11/15] log errors on invited list file operations --- src/invited.rs | 70 ++++++++++++++++++++++++++++++++++++--------- src/op_sync_team.rs | 2 +- 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/invited.rs b/src/invited.rs index cd92f92..f02c922 100644 --- a/src/invited.rs +++ b/src/invited.rs @@ -5,40 +5,78 @@ use std::fs::{File, OpenOptions}; use std::io::{BufRead, BufReader, Write}; use std::path::Path; -#[cfg_attr(test, derive(Debug, PartialEq))] +#[cfg_attr(test, derive(Debug))] pub struct Invited { invited: HashSet, + logger: slog::Logger, +} + +#[cfg(test)] +impl PartialEq for Invited { + fn eq(&self, other: &Self) -> bool { + self.invited == other.invited + } } impl Invited { #[cfg(test)] - pub fn new() -> Invited { + pub fn new(logger: slog::Logger) -> Invited { Invited { invited: HashSet::new(), + logger, } } - pub fn load(path: &Path) -> Result { + pub fn load(logger: slog::Logger, path: &Path) -> Result { // we want to create the file if it doesn't exist even though we won't // be writing to it, this just makes the API easier to use. let file = OpenOptions::new() .read(true) .write(true) .create(true) - .open(path)?; + .open(path) + .map_err(|err| { + error!( + logger, + "Failed to open invited list file {:?}: {:?}", path, err + ); + err + })?; let lines = BufReader::new(file).lines(); let mut invited = HashSet::new(); for line in lines { - invited.insert(GitHubID::new(line?.parse()?)); + let line = line.map_err(|err| { + error!( + logger, + "Failed to read line from invited list file {:?}: {:?}", path, err + ); + err + })?; + + let id = line.parse().map_err(|err| { + error!( + logger, + "Failed to parse invited maintainer github id: {:?}", err + ); + err + })?; + + invited.insert(GitHubID::new(id)); } - Ok(Invited { invited }) + Ok(Invited { invited, logger }) } pub fn save(&self, path: &Path) -> Result<(), ExitError> { - let mut file = File::create(path)?; + let mut file = File::create(path).map_err(|err| { + error!( + self.logger, + "Failed to create invited list file {:?}: {:?}", path, err, + ); + err + })?; let string = self .invited @@ -47,7 +85,13 @@ impl Invited { .collect::>() .join("\n"); - file.write_all(string.as_ref())?; + file.write_all(string.as_ref()).map_err(|err| { + error!( + self.logger, + "Failed to write invited list file {:?}: {:?}", path, err + ); + err + })?; Ok(()) } @@ -71,7 +115,7 @@ mod tests { #[test] fn test_load_save() { - let mut invited = Invited::new(); + let mut invited = Invited::new(rfc39::test_logger()); let tmpdir = tempfile::tempdir().unwrap(); let tmpfile = tmpdir.path().join("invited.txt"); @@ -81,7 +125,7 @@ mod tests { invited.save(&tmpfile).unwrap(); - let loaded_invited = Invited::load(&tmpfile).unwrap(); + let loaded_invited = Invited::load(rfc39::test_logger(), &tmpfile).unwrap(); assert_eq!(invited, loaded_invited); } @@ -91,14 +135,14 @@ mod tests { let tmpdir = tempfile::tempdir().unwrap(); let tmpfile = tmpdir.path().join("invited.txt"); - let invited = Invited::load(&tmpfile).unwrap(); + let invited = Invited::load(rfc39::test_logger(), &tmpfile).unwrap(); - assert_eq!(invited, Invited::new()); + assert_eq!(invited, Invited::new(rfc39::test_logger())); } #[test] fn test_add_remove_invited() { - let mut invited = Invited::new(); + let mut invited = Invited::new(rfc39::test_logger()); assert!(!invited.contains(&GitHubID::new(0))); diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index cf09b28..5ce55d1 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -146,7 +146,7 @@ pub fn sync_team( current_team_member_gauge.set(current_members.len().try_into().unwrap()); - let mut invited = Invited::load(&invited_list)?; + let mut invited = Invited::load(logger.clone(), &invited_list)?; debug!(logger, "Fetching existing invitations"); let pending_invites: Vec = rt From 70cd3a5c49e8295208935d978fdbeeac1a816751 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 15:56:59 +0000 Subject: [PATCH 12/15] add metrics for pending invitations and previously invited --- src/op_sync_team.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index 5ce55d1..0b481bf 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -189,6 +189,16 @@ pub fn sync_team( register_int_counter!("rfc39_team_sync_additions", "Total team additions").unwrap(); let removals = register_int_counter!("rfc39_team_sync_removals", "Total team removals").unwrap(); + let pending_invitations = register_int_counter!( + "rfc39_team_sync_invite_pending", + "Total pending team invitations" + ) + .unwrap(); + let previously_invited = register_int_counter!( + "rfc39_team_sync_previously_invited", + "Total users not invited because we know we invited them already" + ) + .unwrap(); let errors = register_int_counter!("rfc39_team_sync_errors", "Total team errors").unwrap(); for (github_id, action) in diff { let logger = logger.new(o!( @@ -197,6 +207,8 @@ pub fn sync_team( "changed" => additions.get() + removals.get(), "additions" => additions.get(), "removals" => removals.get(), + "pending-invitations" => pending_invitations.get(), + "previously-invited" => previously_invited.get(), "noops" => noops.get(), "errors" => errors.get(), )); @@ -215,9 +227,11 @@ pub fn sync_team( if pending_invites.contains(&github_name) { noops.inc(); + pending_invitations.inc(); debug!(logger, "User already has a pending invitation"); } else if invited.contains(&github_id) { noops.inc(); + previously_invited.inc(); debug!(logger, "User was already invited previously (since there's no pending invitation we can assume the user rejected the invite)"); } else { additions.inc(); From 1707d71d54a09bb3fcb25fa4b0c9a53f96f8c86c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 16:04:34 +0000 Subject: [PATCH 13/15] add gauges for invited list loaded / saved --- src/invited.rs | 8 ++++++++ src/op_sync_team.rs | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/invited.rs b/src/invited.rs index f02c922..4d77e3d 100644 --- a/src/invited.rs +++ b/src/invited.rs @@ -96,6 +96,10 @@ impl Invited { Ok(()) } + pub fn len(&self) -> usize { + self.invited.len() + } + pub fn contains(&self, id: &GitHubID) -> bool { self.invited.contains(id) } @@ -127,6 +131,8 @@ mod tests { let loaded_invited = Invited::load(rfc39::test_logger(), &tmpfile).unwrap(); + assert_eq!(invited.len(), loaded_invited.len()); + assert_eq!(invited, loaded_invited); } @@ -147,9 +153,11 @@ mod tests { assert!(!invited.contains(&GitHubID::new(0))); invited.add(GitHubID::new(0)); + assert_eq!(invited.len(), 1); assert!(invited.contains(&GitHubID::new(0))); invited.remove(&GitHubID::new(0)); + assert_eq!(invited.len(), 0); assert!(!invited.contains(&GitHubID::new(0))); } } diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index 0b481bf..91a7d06 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -110,6 +110,18 @@ pub fn sync_team( ) .unwrap(); + let invited_list_loaded_gauge: IntGauge = register_int_gauge!( + "rfc39_invited_list_loaded", + "Number of github ids loaded from the previously invited list" + ) + .unwrap(); + + let invited_list_saved_gauge: IntGauge = register_int_gauge!( + "rfc39_invited_list_saved", + "Number of github ids saved to the previously invited list" + ) + .unwrap(); + let mut rt = TrackedReactor { rt: Runtime::new().unwrap(), }; @@ -147,6 +159,7 @@ pub fn sync_team( current_team_member_gauge.set(current_members.len().try_into().unwrap()); let mut invited = Invited::load(logger.clone(), &invited_list)?; + invited_list_loaded_gauge.set(invited.len().try_into().unwrap()); debug!(logger, "Fetching existing invitations"); let pending_invites: Vec = rt @@ -348,6 +361,7 @@ pub fn sync_team( } invited.save(&invited_list)?; + invited_list_saved_gauge.set(invited.len().try_into().unwrap()); Ok(()) } From 5bfb2e527567e2ccd9859ebaf54de99215b11d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 18:59:23 +0000 Subject: [PATCH 14/15] don't early return when maximum change limit is hit --- src/op_sync_team.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/op_sync_team.rs b/src/op_sync_team.rs index 91a7d06..37a0aac 100644 --- a/src/op_sync_team.rs +++ b/src/op_sync_team.rs @@ -228,7 +228,7 @@ pub fn sync_team( if let Some(limit) = limit { if (additions.get() + removals.get()) >= limit { info!(logger, "Hit maximum change limit"); - return Ok(()); + break; } } match action { From 983081046beb7507e7b7dff846bc7a5d1c5bebd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 28 Dec 2020 19:00:19 +0000 Subject: [PATCH 15/15] sort invited ids when persisting to file --- src/invited.rs | 8 +++++--- src/maintainers.rs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/invited.rs b/src/invited.rs index 4d77e3d..fe916ad 100644 --- a/src/invited.rs +++ b/src/invited.rs @@ -78,9 +78,11 @@ impl Invited { err })?; - let string = self - .invited - .iter() + let mut values = self.invited.iter().collect::>(); + values.sort(); + + let string = values + .into_iter() .map(|id| id.to_string()) .collect::>() .join("\n"); diff --git a/src/maintainers.rs b/src/maintainers.rs index 8896cb6..74bd9b4 100644 --- a/src/maintainers.rs +++ b/src/maintainers.rs @@ -53,7 +53,7 @@ impl GitHubName { } } -#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone, Deserialize)] +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Copy, Clone, Deserialize)] pub struct GitHubID(u64); impl std::fmt::Display for GitHubID { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {