Persist invited users by andresilva · Pull Request #6 · NixOS/rfc39

andresilva · 2020-12-27T22:09:13Z

Fix #3.

An option --invited-list is added which takes a file path where github ids of invited users will be persisted. This is a plain text file where one github id (a number) is stored per line (this makes it easy to manually edit).

andresilva · 2020-12-27T22:11:35Z

I didn't actually test this since the setup to do so isn't easy. I think it should work but would appreciate if someone who has access to the existing infrastructure could run it.

cole-h

Overall LGTM, just a few comments.

Thanks for taking initiative on this! I had been meaning to get around to it, but... y'know, life, work, holidays, yadda-yadda.

src/op_sync_team.rs

andresilva · 2020-12-28T11:09:03Z

I wanted to be added to the NixOS organization and was told that wouldn't happen until this issue was closed 🙈. Thanks for the quick review @cole-h, I think I addressed all comments.

grahamc

Looks pretty good to me, I'll give this a test shortly. Thank you for working on this!

One thing I'm thinking is maybe the Invited struct should receive a logger and print log messages in the error cases which right now just ?: IO errors especially are hard to track down, and parsing can be a bit fraught, though this parsing is super simple. What do you think?

I also think we should probably export a gauge of how many entries are loaded and saved, for monitoring purposes.

Does this sound good? I can help with these things if you'd like!

src/op_sync_team.rs

src/cli.rs

andresilva · 2020-12-28T16:05:28Z

@grahamc Thanks for the review. Just pushed some more changes which I think should address all your suggestions.

grahamc · 2020-12-28T16:32:46Z

It looks like it isn't quite right. Check this out, after this run the invitations list is empty and these two metrics are notable:

# HELP rfc39_invited_list_loaded Number of github ids loaded from the previously invited list
# TYPE rfc39_invited_list_loaded gauge
rfc39_invited_list_loaded 0
# HELP rfc39_invited_list_saved Number of github ids saved to the previously invited list
# TYPE rfc39_invited_list_saved gauge
rfc39_invited_list_saved 0

[nix-shell:~/nixpkgs]$ ../rfc39/result/bin/rfc39 --dump-metrics --metrics-delay=1 --metrics-addr=0.0.0.0:9190 --credentials /run/keys/rfc39-credentials.nix --maintainers ./maintainers/maintainer-list.nix sync-team NixOS 3345117 --limit 1 --invited-list ./invitations
Dec 28 16:29:56.164 INFO Listening on 0.0.0.0:9190, thread: metrics, module: rfc39:223
Dec 28 16:29:56.164 INFO Loading maintainer information, absolute: /root/nixpkgs/maintainers/maintainer-list.nix, from: ./maintainers/maintainer-list.nix, module: rfc39:73
Dec 28 16:29:56.201 INFO Loading GitHub authentication information from "/run/keys/rfc39-credentials.nix", module: rfc39:82
Dec 28 16:29:56.552 INFO Syncing team, team_id: 3345117, team_name: Nixpkgs Maintainers, exec-mode: SyncTeam, module: rfc39::op_sync_team:136
Dec 28 16:29:56.552 INFO Fetching current team members, team_id: 3345117, team_name: Nixpkgs Maintainers, exec-mode: SyncTeam, module: rfc39::op_sync_team:141
Dec 28 16:30:10.839 INFO Adding user to the team, github-name: thelegy, nixpkgs-handle: thelegy, errors: 0, noops: 12, previously-invited: 0, pending-invitations: 0, removals: 0, additions: 0, changed: 0, github-id: 3105057, dry-run: false, exec-mode: SyncTeam, module: rfc39::op_sync_team:251
Dec 28 16:30:11.285 INFO Hit maximum change limit, errors: 0, noops: 12, previously-invited: 0, pending-invitations: 0, removals: 0, additions: 1, changed: 1, github-id: 311580, dry-run: false, exec-mode: SyncTeam, module: rfc39::op_sync_team:230
metrics:
[...snip...]

grahamc · 2020-12-28T16:40:54Z

It looks like the problem is this early return:

rfc39/src/op_sync_team.rs

Lines 198 to 202 in c44ecfb

    
           if let Some(limit) = limit { 
        
               if (additions.get() + removals.get()) >= limit { 
        
                   info!(logger, "Hit maximum change limit"); 
        
                   return Ok(()); 
        
               }

we probably want to make sure the invitations are saved no matter how we exit this, perhaps breaking the body of the loop out in to another function, and in the existing function call save in all cases?

rfc39/src/op_sync_team.rs

Lines 188 to 316 in c44ecfb

    
           for (github_id, action) in diff { 
        
               let logger = logger.new(o!( 
        
                   "dry-run" => dry_run, 
        
                   "github-id" => format!("{}", github_id), 
        
                   "changed" => additions.get() + removals.get(), 
        
                   "additions" => additions.get(), 
        
                   "removals" => removals.get(), 
        
                   "noops" => noops.get(), 
        
                   "errors" => errors.get(), 
        
               )); 
        
               if let Some(limit) = limit { 
        
                   if (additions.get() + removals.get()) >= limit { 
        
                       info!(logger, "Hit maximum change limit"); 
        
                       return Ok(()); 
        
                   } 
        
               } 
        
               match action { 
        
                   TeamAction::Add(github_name, 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 { 
        
                           additions.inc(); 
        
                           info!(logger, "Adding user to the team"); 
        
                           if do_it_live { 
        
                               // verify the ID and name still match 
        
                               let get_user = rt.block_on( 
        
                                   github.users().get(&format!("{}", github_name)), 
        
                                   &github_get_user_histogram, 
        
                                   &github_get_user_failures, 
        
                               ) 
        
                                   .map_err(|e| { 
        
                                       errors.inc(); 
        
                                       warn!(logger, "Failed to fetch user by name, incrementing noops. error: {:#?}", e); 
        
                                       e 
        
                                   }) 
        
                                   .map(|user| { 
        
                                       if GitHubID::new(user.id) != github_id { 
        
                                           github_user_unchanged_username_id_mismatch.inc(); 
        
                                           warn!(logger, "Recorded username mismatch, not adding"); 
        
                                           None 
        
                                       } else { 
        
                                           Some(user) 
        
                                       } 
        
                                   }); 
        
                               if let Ok(Some(_user)) = get_user { 
        
                                   let add_attempt = rt.block_on( 
        
                                       team_actions.add_user( 
        
                                           &format!("{}", github_name), 
        
                                           TeamMemberOptions { 
        
                                               role: TeamMemberRole::Member, 
        
                                           }, 
        
                                       ), 
        
                                       &github_add_user_histogram, 
        
                                       &github_add_user_failures, 
        
                                   ); 
        
                                   match add_attempt { 
        
                                       Ok(_) => (), 
        
                                       Err(e) => { 
        
                                           errors.inc(); 
        
                                           warn!(logger, "Failed to add a user to the team, not decrementing additions as it may have succeeded: {:#?}", e 
        
                                           ); 
        
                                       } 
        
                                   } 
        
                               } 
        
                           } 
        
                       } 
        
                   } 
        
                   TeamAction::Keep(handle) => { 
        
                       let logger = logger.new(o!( 
        
                           "nixpkgs-handle" => format!("{}", handle), 
        
                       )); 
        
                       noops.inc(); 
        
                       trace!(logger, "Keeping user on the team"); 
        
                   } 
        
                   TeamAction::Remove(github_name) => { 
        
                       let logger = logger.new(o!( 
        
                           "github-name" => format!("{}", github_name),                )); 
        
                       removals.inc(); 
        
                       info!(logger, "Removing user from the team"); 
        
                       if do_it_live { 
        
                           // verify the ID and name still match 
        
                           let get_user = rt 
        
                               .block_on( 
        
                                   github.users().get(&format!("{}", github_name)), 
        
                                   &github_get_user_histogram, 
        
                                   &github_get_user_failures, 
        
                               ) 
        
                               .map_err(|e| { 
        
                                   errors.inc(); 
        
                                   warn!( 
        
                                       logger, 
        
                                       "Failed to fetch user by name, incrementing noops. error: {:#?}", e 
        
                                   ); 
        
                                   e 
        
                               }) 
        
                               .map(|user| { 
        
                                   if GitHubID::new(user.id) != github_id { 
        
                                       github_user_unchanged_username_id_mismatch.inc(); 
        
                                       warn!(logger, "Recorded username mismatch, not adding"); 
        
                                       None 
        
                                   } else { 
        
                                       Some(user) 
        
                                   } 
        
                               }); 
        
                           if let Ok(Some(_user)) = get_user { 
        
                               if let Err(e) = 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); 
        
                               } 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
           }

I'm open to many options here, and again I'm happy to help.

grahamc · 2020-12-28T18:18:50Z

Locally I applied this diff:

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 {

and am seeing saves correctly. However, I noticed the order is not stable,

[nix-shell:~/nixpkgs]# cat invitations
3105057
335406

[nix-shell:~/nixpkgs]# cat invitations
335406
1699466
3105057

[nix-shell:~/nixpkgs]# cat invitations
3105057
335406
1699466
592849

so I further added this patch:

diff --git a/src/invited.rs b/src/invited.rs
index 4d77e3d..aaa7704 100644
--- a/src/invited.rs
+++ b/src/invited.rs
@@ -78,9 +78,13 @@ impl Invited {
             err
         })?;

-        let string = self
+        let mut values = self
             .invited
             .iter()
+            .collect::<Vec<_>>();
+        values.sort();
+        let string = values
+            .into_iter()
             .map(|id| id.to_string())
             .collect::<Vec<_>>()
             .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 {

and it is looking good from here:

[nix-shell:~/nixpkgs]# cat invitations
335406
592849
1699466
2379774
3105057
6067895
12898828

What do you think about these two patches?

cole-h · 2020-12-28T18:40:48Z

LGTM.

andresilva · 2020-12-28T19:01:35Z

@grahamc I applied both your suggestions! 👍 Another alternative for the unstable ordering would have been to just use a BTreeSet instead of a HashSet.

grahamc · 2020-12-28T20:11:26Z

Thanks, merged and applied: NixOS/rfc39-record@d6bca12 and it is working. Thank you!

grahamc · 2020-12-28T20:13:48Z

Graphs: https://status.nixos.org/grafana/d/w8HHOCBWz/rfc39-maintainer-sync?orgId=1&refresh=30s

andresilva added 4 commits December 27, 2020 21:17

add struct to track invited github ids

fa3aed1

track previously invited users

cc30e34

fix tests

3a1744b

add docs to --invited-list

200987a

andresilva added 3 commits December 27, 2020 22:17

rename Invites to Invited

9624a6e

create invited file if it doesn't exist

70a57e2

parse invited list option with from_os_str

25ce51c

andresilva mentioned this pull request Dec 27, 2020

Create a database of already-invited users and don't invite them again #3

Closed

cole-h suggested changes Dec 28, 2020

View reviewed changes

src/op_sync_team.rs Outdated Show resolved Hide resolved

src/op_sync_team.rs Outdated Show resolved Hide resolved

src/op_sync_team.rs Outdated Show resolved Hide resolved

andresilva added 2 commits December 28, 2020 11:04

only add/remove invited if successful

a99fa5f

distinguish between pending invite / rejected invite

0752d20

grahamc reviewed Dec 28, 2020

View reviewed changes

src/op_sync_team.rs Show resolved Hide resolved

grahamc reviewed Dec 28, 2020

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

andresilva added 4 commits December 28, 2020 15:39

make invited list file mandatory

b796257

log errors on invited list file operations

c273455

add metrics for pending invitations and previously invited

70cd3a5

add gauges for invited list loaded / saved

1707d71

grahamc mentioned this pull request Dec 28, 2020

NOTICE: Invitations to join NixOS Organization are sending again. NixOS/nixpkgs#107815

Closed

andresilva added 2 commits December 28, 2020 18:59

don't early return when maximum change limit is hit

5bfb2e5

sort invited ids when persisting to file

9830810

grahamc merged commit e06e1a5 into NixOS:master Dec 28, 2020

andresilva mentioned this pull request Dec 14, 2021

[2018-2024] Nixpkgs committer nominations NixOS/nixpkgs-committers#60

Closed

Uh oh!

Conversation

andresilva commented Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andresilva commented Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cole-h left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andresilva commented Dec 28, 2020

Uh oh!

grahamc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andresilva commented Dec 28, 2020

Uh oh!

grahamc commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grahamc commented Dec 28, 2020

Uh oh!

grahamc commented Dec 28, 2020

Uh oh!

cole-h commented Dec 28, 2020

Uh oh!

andresilva commented Dec 28, 2020

Uh oh!

grahamc commented Dec 28, 2020

Uh oh!

grahamc commented Dec 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andresilva commented Dec 27, 2020 •

edited

Loading

andresilva commented Dec 27, 2020 •

edited

Loading

grahamc commented Dec 28, 2020 •

edited

Loading