Skip to content

refactor: improve environment diffing using BTreeSet #2188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
marcoieni merged 1 commit into from
Dec 16, 2025
+105 −131
Merged

refactor: improve environment diffing using BTreeSet #2188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 75 additions & 107 deletions sync-team/src/github/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::Config;
use crate::github::api::{GithubRead, Login, PushAllowanceActor, RepoPermission, RepoSettings};
use log::debug;
use rust_team_data::v1::{Bot, BranchProtectionMode, MergeBot};
use std::collections::{BTreeMap, HashMap, HashSet};
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use std::fmt::Write;

pub(crate) use self::api::{GitHubApiRead, GitHubWrite, HttpClient};
Expand Down Expand Up @@ -489,87 +489,79 @@ impl SyncGitHub {
&self,
expected_repo: &rust_team_data::v1::Repo,
) -> anyhow::Result<Vec<EnvironmentDiff>> {
let actual_environments = self
let mut environment_diffs = Vec::new();

let actual_environments_map = self
.github
.repo_environments(&expected_repo.org, &expected_repo.name)?;

let expected_env_names: HashSet<String> =
let actual_environments: BTreeSet<String> =
actual_environments_map.keys().cloned().collect();
let expected_environments: BTreeSet<String> =
expected_repo.environments.keys().cloned().collect();
let actual_env_names: HashSet<String> = actual_environments.keys().cloned().collect();

let mut environment_diffs = Vec::new();
// Environments to create (already sorted via BTreeSet)
for env_name in expected_environments.difference(&actual_environments) {
let env = expected_repo.environments.get(env_name).unwrap();
environment_diffs.push(EnvironmentDiff::Create(env_name.clone(), env.clone()));
}

// Process environments to create or update (sorted for deterministic output)
let mut environments_to_process: Vec<_> = expected_repo.environments.iter().collect();
environments_to_process.sort_by_key(|(name, _)| name.as_str());

for (env_name, expected_env) in environments_to_process {
match actual_environments.get(env_name) {
Some(actual_env) => {
// Skip if both branches and tags are identical (order-independent comparison)
if patterns_equal(&actual_env.branches, &expected_env.branches)
&& patterns_equal(&actual_env.tags, &expected_env.tags)
{
continue;
}
// Environments to update (already sorted via BTreeSet)
for env_name in expected_environments.intersection(&actual_environments) {
let expected_env = expected_repo.environments.get(env_name).unwrap();
let actual_env = actual_environments_map.get(env_name).unwrap();

// Environment exists but patterns differ - compute what to add/remove for branches
let old_branches: HashSet<_> = actual_env.branches.iter().collect();
let new_branches: HashSet<_> = expected_env.branches.iter().collect();

let mut add_branches: Vec<_> = new_branches
.difference(&old_branches)
.map(|&s| s.clone())
.collect();
let mut remove_branches: Vec<_> = old_branches
.difference(&new_branches)
.map(|&s| s.clone())
.collect();

// Compute what to add/remove for tags
let old_tags: HashSet<_> = actual_env.tags.iter().collect();
let new_tags: HashSet<_> = expected_env.tags.iter().collect();

let mut add_tags: Vec<_> =
new_tags.difference(&old_tags).map(|&s| s.clone()).collect();
let mut remove_tags: Vec<_> =
old_tags.difference(&new_tags).map(|&s| s.clone()).collect();

// Sort for deterministic output
add_branches.sort();
remove_branches.sort();
add_tags.sort();
remove_tags.sort();

environment_diffs.push(EnvironmentDiff::Update {
name: env_name.clone(),
add_branches,
remove_branches,
add_tags,
remove_tags,
new_branches: expected_env.branches.clone(),
new_tags: expected_env.tags.clone(),
});
}
None => {
// Environment doesn't exist - create it
environment_diffs.push(EnvironmentDiff::Create {
name: env_name.clone(),
branches: expected_env.branches.clone(),
tags: expected_env.tags.clone(),
});
}
let expected_branches: BTreeSet<_> = expected_env.branches.iter().collect();
let actual_branches: BTreeSet<_> = actual_env.branches.iter().collect();

let expected_tags: BTreeSet<_> = expected_env.tags.iter().collect();
let actual_tags: BTreeSet<_> = actual_env.tags.iter().collect();

let add_branches: Vec<_> = expected_branches
.difference(&actual_branches)
.map(|s| s.to_string())
.collect();

let remove_branches: Vec<_> = actual_branches
.difference(&expected_branches)
.map(|s| s.to_string())
.collect();

let add_tags: Vec<_> = expected_tags
.difference(&actual_tags)
.map(|s| s.to_string())
.collect();

let remove_tags: Vec<_> = actual_tags
.difference(&expected_tags)
.map(|s| s.to_string())
.collect();

if !add_branches.is_empty()
|| !remove_branches.is_empty()
|| !add_tags.is_empty()
|| !remove_tags.is_empty()
{
let mut new_branches = expected_env.branches.clone();
new_branches.sort();
let mut new_tags = expected_env.tags.clone();
new_tags.sort();

environment_diffs.push(EnvironmentDiff::Update {
name: env_name.clone(),
add_branches,
remove_branches,
add_tags,
remove_tags,
new_branches,
new_tags,
});
}
}

// Process environments to delete (sorted for deterministic output)
let mut envs_to_delete: Vec<_> = actual_env_names
.difference(&expected_env_names)
.cloned()
.collect();
envs_to_delete.sort();
for env in envs_to_delete {
environment_diffs.push(EnvironmentDiff::Delete { name: env });
// Environments to delete (already sorted via BTreeSet)
for env_name in actual_environments.difference(&expected_environments) {
environment_diffs.push(EnvironmentDiff::Delete(env_name.clone()));
}

Ok(environment_diffs)
Expand All @@ -588,16 +580,6 @@ impl SyncGitHub {
}
}

/// Compare two string lists for equality, ignoring order
fn patterns_equal(a: &[String], b: &[String]) -> bool {
if a.len() != b.len() {
return false;
}
let a_set: HashSet<&String> = a.iter().collect();
let b_set: HashSet<&String> = b.iter().collect();
a_set == b_set
}

fn calculate_permission_diffs(
expected_repo: &rust_team_data::v1::Repo,
mut actual_teams: HashMap<String, api::RepoTeam>,
Expand Down Expand Up @@ -1013,11 +995,7 @@ struct UpdateRepoDiff {

#[derive(Debug)]
enum EnvironmentDiff {
Create {
name: String,
branches: Vec<String>,
tags: Vec<String>,
},
Create(String, rust_team_data::v1::Environment),
Update {
name: String,
add_branches: Vec<String>,
Expand All @@ -1027,9 +1005,7 @@ enum EnvironmentDiff {
new_branches: Vec<String>,
Copy link
Member

@Kobzol Kobzol Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be better if the sync code directly operated on branches/tags to add or remove, rather than having generic code that then tries to load the current state from GitHub again when doing the sync.

In other words, I would remove new_branches and new_tags, and keep only add/remove.

Copy link
Member

@marcoieni marcoieni Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approved this PR because it's an improvement with respect to the previous code.
This improvement can be done in another PR 👍

new_tags: Vec<String>,
},
Delete {
name: String,
},
Delete(String),
}

impl UpdateRepoDiff {
Expand Down Expand Up @@ -1089,12 +1065,8 @@ impl UpdateRepoDiff {

for env_diff in &self.environment_diffs {
match env_diff {
EnvironmentDiff::Create {
name,
branches,
tags,
} => {
sync.create_environment(&self.org, &self.name, name, branches, tags)?;
EnvironmentDiff::Create(name, env) => {
sync.create_environment(&self.org, &self.name, name, &env.branches, &env.tags)?;
}
EnvironmentDiff::Update {
name,
Expand All @@ -1107,7 +1079,7 @@ impl UpdateRepoDiff {
} => {
sync.update_environment(&self.org, &self.name, name, new_branches, new_tags)?;
}
EnvironmentDiff::Delete { name } => {
EnvironmentDiff::Delete(name) => {
sync.delete_environment(&self.org, &self.name, name)?;
}
}
Expand Down Expand Up @@ -1186,17 +1158,13 @@ impl std::fmt::Display for UpdateRepoDiff {
writeln!(f, " Environments:")?;
for env_diff in environment_diffs {
match env_diff {
EnvironmentDiff::Create {
name,
branches,
tags,
} => {
EnvironmentDiff::Create(name, env) => {
writeln!(f, " ➕ Create: {name}")?;
if !branches.is_empty() {
writeln!(f, " Branches: {}", branches.join(", "))?;
if !env.branches.is_empty() {
writeln!(f, " Branches: {}", env.branches.join(", "))?;
}
if !tags.is_empty() {
writeln!(f, " Tags: {}", tags.join(", "))?;
if !env.tags.is_empty() {
writeln!(f, " Tags: {}", env.tags.join(", "))?;
}
}
EnvironmentDiff::Update {
Expand Down Expand Up @@ -1233,7 +1201,7 @@ impl std::fmt::Display for UpdateRepoDiff {
writeln!(f, " No pattern changes")?;
}
}
EnvironmentDiff::Delete { name } => writeln!(f, " ❌ Delete: {name}")?,
EnvironmentDiff::Delete(name) => writeln!(f, " ❌ Delete: {name}")?,
}
}
}
Expand Down
54 changes: 30 additions & 24 deletions sync-team/src/github/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,16 +990,20 @@ fn repo_environment_create() {
permission_diffs: [],
branch_protection_diffs: [],
environment_diffs: [
Create {
name: "production",
branches: [],
tags: [],
},
Create {
name: "staging",
branches: [],
tags: [],
},
Create(
"production",
Environment {
branches: [],
tags: [],
},
),
Create(
"staging",
Environment {
branches: [],
tags: [],
},
),
],
},
),
Expand Down Expand Up @@ -1044,12 +1048,12 @@ fn repo_environment_delete() {
permission_diffs: [],
branch_protection_diffs: [],
environment_diffs: [
Delete {
name: "production",
},
Delete {
name: "staging",
},
Delete(
"production",
),
Delete(
"staging",
),
],
},
),
Expand Down Expand Up @@ -1109,14 +1113,16 @@ fn repo_environment_update() {
permission_diffs: [],
branch_protection_diffs: [],
environment_diffs: [
Create {
name: "dev",
branches: [],
tags: [],
},
Delete {
name: "staging",
},
Create(
"dev",
Environment {
branches: [],
tags: [],
},
),
Delete(
"staging",
),
],
},
),
Expand Down