gws: add diff for google accounts by ubiratansoares · Pull Request #2445 · rust-lang/team

ubiratansoares · 2026-04-23T16:52:55Z

First PR to enable diffs betwen our configuration and Google Workspace state.

The whole feature would be a much bigger PR, so I decided to split it. This PR looks big but it brings a non negletable amount of boilterplate, so hopefully it is not hard to read. I hope the code style is familiar enough compared with other services we also diff and sync.

For now, this PR adds support for diffing google workspace users, supporting creation and deletion operations. Everything is still detached from the app entrypoint, and integration will come after we are able to run live dry-runs in CI (which requires further configuration on Github Actions).

To make it sure we can define and safely test some rules we want to encode as part of the diff logic, I added a fake GWS API similar approach similar to what we use in github.

Towards #2363

github-actions · 2026-04-23T16:56:18Z

Dry-run check results


[WARN  rust_team::sync] sync-team is running in dry mode, no changes will be applied.
[INFO  rust_team::sync] synchronizing crates-io
[INFO  rust_team::sync] synchronizing github

rustbot · 2026-04-28T06:54:30Z

rust_team_data/src/v1.rs has been modified, it is used (as a git dependency) by multiple sub-projects like triagebot, the www.rust-lang.org website and others.

If you are changing the data structures, please make sure that the changes are not going to break serde deserialization (adding a field is fine; removing or renaming a field isn't).

If you must do a breaking change to the format, make sure to coordinate it with all the users of the rust_team_data crate.

marcoieni · 2026-04-28T10:07:05Z

                }
            }
+            "google-workspace" => {
+                println!("Nothing to diff");


Suggested change

println!("Nothing to diff");

println!("google-workspace: Nothing to diff");

Otherwise, CI will print just "Nothing to diff" and that's confusing.

Btw I assume the diff itself will be implemented in a later PR 👍

Indeed, but happy to change it to make it clear

marcoieni · 2026-04-28T10:25:07Z

    pub is_lead: bool,
    #[serde(skip_serializing_if = "Vec::is_empty", default)]
    pub roles: Vec<String>,
+    pub google_workspace: Option<GoogleWorkspace>,


Suggested change

pub google_workspace: Option<GoogleWorkspace>,

#[serde(skip_serializing_if = "Option::is_none")]

pub google_workspace: Option<GoogleWorkspace>,

Otherwise the response contains "google_workspace": null

marcoieni · 2026-04-28T10:33:41Z

+            .filter_map(|team| match team.google_workspace_saml_group {
+                None => None,
+                Some(opt_in) => {
+                    if opt_in {
+                        Some(&team.members)
+                    } else {
+                        None
+                    }
+                }
+            })
+            .flatten()


Suggested change

.filter_map(|team| match team.google_workspace_saml_group {

None => None,

Some(opt_in) => {

if opt_in {

Some(&team.members)

} else {

None

}

}

})

.flatten()

.filter(|team| team.google_workspace_saml_group.unwrap_or_default())

.flat_map(|team| &team.members)

obtained by just asking codex 5.5 low reasoning "make this more idiomatic"

marcoieni · 2026-04-28T10:39:14Z

+pub struct GoogleWorkspace {
+    pub name: String,
+    pub surname: String,
+    pub account_handle: String,


why do we have a mismatch between these fields and the fields in the schema?
Isn't it easier if we call them the same? 🤔

No particular reason, I can make them equal for simplicity! :)

marcoieni · 2026-04-28T10:41:57Z

+                        GoogleWorkspace {
+                            name: gws.first_name,
+                            surname: gws.last_name,
+                            account_handle: gws.account_handle,


this code is duplicated from above. Maybe we can have a From implementation or something like this?

marcoieni · 2026-04-28T10:42:27Z

                    github_id: person.github_id(),
                    is_lead: leads.contains(github_name),
                    roles: website_roles.get(*github_name).cloned().unwrap_or_default(),
+                    google_workspace: person.google_workspace().clone().map(|gws| {


I noticed that the function google_workspace() returns &Option<Something>, but Option<&Something> is more idiomatic.

We could change the function like this:

pub(crate) fn google_workspace(&self) -> Option<&GoogleWorkspace> { self.google_workspace.as_ref() }

marcoieni · 2026-04-28T10:52:17Z

+        users.iter().any(|account| account.primary_email == email)
+    }
+
+    fn matches_by_saml_group(&self, target: &Group, groups: &[Group]) -> bool {


this function is unused

marcoieni · 2026-04-28T10:57:08Z

+        let declared_users = members_from_gws_enabled_teams
+            .iter()
+            .filter_map(|member| member.google_workspace.as_ref().map(User::from))
+            .collect::<Vec<_>>();


Suggested change

let declared_users = members_from_gws_enabled_teams

.iter()

.filter_map(|member| member.google_workspace.as_ref().map(User::from))

.collect::<Vec<_>>();

let mut seen_primary_emails = BTreeSet::new();

let declared_users = members_from_gws_enabled_teams

.iter()

.filter_map(|member| member.google_workspace.as_ref().map(User::from))

.filter(|user| seen_primary_emails.insert(user.primary_email.clone()))

.collect::<Vec<_>>();

Should we deduplicated by some kind of criteria (eg email)?
So that we don't try to create users multiple times?

I did not focused on dedup because, in practice, the number of users will be quite small (~ 30 after everyone onboarded). But happy to try a better approach. Commit to follow

gws: add diff for google accounts

af352dc

ubiratansoares self-assigned this Apr 23, 2026

ubiratansoares marked this pull request as ready for review April 28, 2026 06:54

ubiratansoares requested review from Mark-Simulacrum, emilyalbini, jdno and marcoieni as code owners April 28, 2026 06:54

marcoieni reviewed Apr 28, 2026

View reviewed changes

jieyouxu added needs-infra-admin-review This change requires one of the `infra-admins` to review. S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin. labels May 2, 2026

	println!("Nothing to diff");
	println!("google-workspace: Nothing to diff");

	pub google_workspace: Option<GoogleWorkspace>,
	#[serde(skip_serializing_if = "Option::is_none")]
	pub google_workspace: Option<GoogleWorkspace>,

Conversation

ubiratansoares commented Apr 23, 2026

Uh oh!

github-actions Bot commented Apr 23, 2026

Uh oh!

rustbot commented Apr 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants