Persist invited users

[andresilva]

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]

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.

[andresilva]

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!

[andresilva]

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

[grahamc]

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]

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]

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]

LGTM.

[andresilva]

@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]

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

[grahamc]

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