Persist invited users

src/cli.rs

@@ -70,6 +70,12 @@ pub struct SyncTeamParams {
 
     #[structopt(long = "limit")]
     pub limit: Option<u64>,
+
+    /// 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", parse(from_os_str))]
+    pub invited_list: Option<PathBuf>,
 }
 
 #[derive(Debug, StructOpt)]
@@ -80,13 +86,22 @@ pub struct ListTeamParams {
 #[derive(Debug)]
 pub enum ExitError {
     Io(std::io::Error),
+    InvalidGitHubID(std::num::ParseIntError),
     Serde(serde_json::error::Error),
 }
+
 impl From<std::io::Error> for ExitError {
     fn from(e: std::io::Error) -> Self {
         Self::Io(e)
     }
 }
+
+impl From<std::num::ParseIntError> for ExitError {
+    fn from(e: std::num::ParseIntError) -> Self {
+        Self::InvalidGitHubID(e)
+    }
+}
+
 impl From<serde_json::error::Error> for ExitError {
     fn from(e: serde_json::error::Error) -> Self {
         Self::Serde(e)

src/invited.rs

@@ -0,0 +1,110 @@
+use crate::cli::ExitError;
+use crate::maintainers::GitHubID;
+use std::collections::HashSet;
+use std::fs::{File, OpenOptions};
+use std::io::{BufRead, BufReader, Write};
+use std::path::Path;
+
+#[cfg_attr(test, derive(Debug, PartialEq))]
+pub struct Invited {
+    invited: HashSet<GitHubID>,
+}
+
+impl Invited {
+    pub fn new() -> Invited {
+        Invited {
+            invited: HashSet::new(),
+        }
+    }
+
+    pub fn load(path: &Path) -> Result<Invited, ExitError> {
+        // 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();
+        for line in lines {
+            invited.insert(GitHubID::new(line?.parse()?));
+        }
+
+        Ok(Invited { 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::<Vec<_>>()
+            .join("\n");
+
+        file.write_all(string.as_ref())?;
+
+        Ok(())
+    }
+
+    pub fn contains(&self, id: &GitHubID) -> bool {
+        self.invited.contains(id)
+    }
+
+    pub fn add(&mut self, id: GitHubID) {
+        self.invited.insert(id);
+    }
+
+    pub fn remove(&mut self, id: &GitHubID) {
+        self.invited.remove(id);
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_load_save() {
+        let mut invited = Invited::new();
+        let tmpdir = tempfile::tempdir().unwrap();
+        let tmpfile = tmpdir.path().join("invited.txt");
+
+        for n in (0..20).step_by(3) {
+            invited.add(GitHubID::new(n));
+        }
+
+        invited.save(&tmpfile).unwrap();
+
+        let loaded_invited = Invited::load(&tmpfile).unwrap();
+
+        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();
+
+        assert!(!invited.contains(&GitHubID::new(0)));
+
+        invited.add(GitHubID::new(0));
+        assert!(invited.contains(&GitHubID::new(0)));
+
+        invited.remove(&GitHubID::new(0));
+        assert!(!invited.contains(&GitHubID::new(0)));
+    }
+}

src/main.rs

@@ -23,6 +23,7 @@ use std::path::{Path, PathBuf};
 use structopt::StructOpt;
 mod cli;
 use cli::{ExecMode, ExitError, Options};
+mod invited;
 mod maintainers;
 use maintainers::MaintainerList;
 mod filemunge;
@@ -178,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),
     }

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";

src/op_sync_team.rs

@@ -1,11 +1,13 @@
 use crate::cli::ExitError;
+use crate::invited::Invited;
 use crate::maintainers::{GitHubID, GitHubName, Handle, MaintainerList};
 use futures::stream::Stream;
 use hubcaps::teams::{TeamMemberOptions, TeamMemberRole};
 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<u64>,
+    invited_list: Option<PathBuf>,
 ) -> 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 invited = if let Some(ref invited_list) = invited_list {
+        Invited::load(invited_list)?
+    } else {
+        Invited::new()
+    };
+
     debug!(logger, "Fetching existing invitations");
     let pending_invites: Vec<GitHubName> = rt
         .block_on(
@@ -202,14 +211,18 @@ 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) {
                     noops.inc();
                     debug!(logger, "User already has a pending invitation");
+                } else if invited.contains(&github_id) {
+                    noops.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();
                     info!(logger, "Adding user to the team");
@@ -249,11 +262,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);
                                 }
                             }
                         }
@@ -268,7 +285,7 @@ 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),                ));
 
@@ -300,21 +317,30 @@ 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);
+                            }
                         }
                     }
                 }
             }
         }
     }
 
+    if let Some(ref invited_list) = invited_list {
+        invited.save(invited_list)?;
+    }
+
     Ok(())
 }
 
@@ -345,8 +371,8 @@ impl TrackedReactor {
 
 #[derive(Debug, PartialEq)]
 enum TeamAction {
-    Add(GitHubName, Handle),
-    Remove(GitHubName),
+    Add(GitHubName, GitHubID, Handle),
+    Remove(GitHubName, GitHubID),
     Keep(Handle),
 }
 
@@ -379,7 +405,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 +417,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),
+            );
         }
     }
 
@@ -438,12 +470,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()