refactor: code clean up

Author: amustaque97

sync-team/src/github/api/read.rs

@@ -1,3 +1,4 @@
+use crate::github::api::Ruleset;
 use crate::github::api::{
     BranchProtection, GraphNode, GraphNodes, GraphPageInfo, HttpClient, Login, Repo, RepoTeam,
     RepoUser, Team, TeamMember, TeamRole, team_node_id, url::GitHubUrl, user_node_id,
@@ -550,8 +551,6 @@ impl GithubRead for GitHubApiRead {
         org: &str,
         repo: &str,
     ) -> anyhow::Result<Vec<crate::github::api::Ruleset>> {
-        use crate::github::api::Ruleset;
-
         #[derive(serde::Deserialize)]
         struct RulesetsResponse {
             #[serde(default)]

sync-team/src/github/api/write.rs

@@ -517,6 +517,23 @@ impl GitHubWrite {
         );
         self.upsert_environment(org, repo, name, branches, tags)
     }
+
+    /// Update an environment in a repository
+    pub(crate) fn update_environment(
+        &self,
+        org: &str,
+        repo: &str,
+        name: &str,
+        branches: &[String],
+        tags: &[String],
+    ) -> anyhow::Result<()> {
+        debug!(
+            "Updating environment '{name}' in '{org}/{repo}' with branches: {:?}, tags: {:?}",
+            branches, tags
+        );
+        self.upsert_environment(org, repo, name, branches, tags)
+    }
+
     /// Internal helper to create or update an environment
     fn upsert_environment(
         &self,

sync-team/src/github/mod.rs

@@ -390,10 +390,8 @@ impl SyncGitHub {
 
         let permission_diffs = self.diff_permissions(expected_repo)?;
 
-        // Always diff branch protections
         let branch_protection_diffs = self.diff_branch_protections(&actual_repo, expected_repo)?;
 
-        // Additionally diff rulesets if configured
         let ruleset_diffs = if self.should_use_rulesets(expected_repo) {
             self.diff_rulesets(expected_repo)?
         } else {
@@ -520,12 +518,12 @@ impl SyncGitHub {
     ) -> anyhow::Result<Vec<EnvironmentDiff>> {
         let mut environment_diffs = Vec::new();
 
-        let actual_environments: HashSet<String> = self
+        let actual_environments_map = self
             .github
-            .repo_environments(&expected_repo.org, &expected_repo.name)?
-            .into_keys()
-            .collect();
+            .repo_environments(&expected_repo.org, &expected_repo.name)?;
 
+        let actual_environments: HashSet<String> =
+            actual_environments_map.keys().cloned().collect();
         let expected_environments: HashSet<String> =
             expected_repo.environments.keys().cloned().collect();
 
@@ -540,6 +538,68 @@ impl SyncGitHub {
             environment_diffs.push(EnvironmentDiff::Create(env_name, env.clone()));
         }
 
+        // Environments to update (sorted for deterministic output)
+        let mut to_update: Vec<_> = expected_environments
+            .intersection(&actual_environments)
+            .cloned()
+            .collect();
+        to_update.sort();
+        for env_name in to_update {
+            let expected_env = expected_repo.environments.get(&env_name).unwrap();
+            let actual_env = actual_environments_map.get(&env_name).unwrap();
+
+            let expected_branches: HashSet<_> = expected_env.branches.iter().collect();
+            let actual_branches: HashSet<_> = actual_env.branches.iter().collect();
+
+            let expected_tags: HashSet<_> = expected_env.tags.iter().collect();
+            let actual_tags: HashSet<_> = actual_env.tags.iter().collect();
+
+            let mut add_branches: Vec<_> = expected_branches
+                .difference(&actual_branches)
+                .map(|s| s.to_string())
+                .collect();
+            add_branches.sort();
+
+            let mut remove_branches: Vec<_> = actual_branches
+                .difference(&expected_branches)
+                .map(|s| s.to_string())
+                .collect();
+            remove_branches.sort();
+
+            let mut add_tags: Vec<_> = expected_tags
+                .difference(&actual_tags)
+                .map(|s| s.to_string())
+                .collect();
+            add_tags.sort();
+
+            let mut remove_tags: Vec<_> = actual_tags
+                .difference(&expected_tags)
+                .map(|s| s.to_string())
+                .collect();
+            remove_tags.sort();
+
+            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,
+                    add_branches,
+                    remove_branches,
+                    add_tags,
+                    remove_tags,
+                    new_branches,
+                    new_tags,
+                });
+            }
+        }
+
         // Environments to delete (sorted for deterministic output)
         let mut to_delete: Vec<_> = actual_environments
             .difference(&expected_environments)
@@ -1161,6 +1221,15 @@ struct UpdateRepoDiff {
 #[derive(Debug)]
 enum EnvironmentDiff {
     Create(String, rust_team_data::v1::Environment),
+    Update {
+        name: String,
+        add_branches: Vec<String>,
+        remove_branches: Vec<String>,
+        add_tags: Vec<String>,
+        remove_tags: Vec<String>,
+        new_branches: Vec<String>,
+        new_tags: Vec<String>,
+    },
     Delete(String),
 }
 
@@ -1230,6 +1299,14 @@ impl UpdateRepoDiff {
                 EnvironmentDiff::Create(name, env) => {
                     sync.create_environment(&self.org, &self.name, name, &env.branches, &env.tags)?;
                 }
+                EnvironmentDiff::Update {
+                    name,
+                    new_branches,
+                    new_tags,
+                    ..
+                } => {
+                    sync.update_environment(&self.org, &self.name, name, new_branches, new_tags)?;
+                }
                 EnvironmentDiff::Delete(name) => {
                     sync.delete_environment(&self.org, &self.name, name)?;
                 }
@@ -1325,6 +1402,40 @@ impl std::fmt::Display for UpdateRepoDiff {
                             writeln!(f, "        Tags: {}", env.tags.join(", "))?;
                         }
                     }
+                    EnvironmentDiff::Update {
+                        name,
+                        add_branches,
+                        remove_branches,
+                        add_tags,
+                        remove_tags,
+                        new_branches: _,
+                        new_tags: _,
+                    } => {
+                        writeln!(f, "    🔄 Update: {name}")?;
+                        if !add_branches.is_empty() {
+                            writeln!(f, "        Adding branches: {}", add_branches.join(", "))?;
+                        }
+                        if !remove_branches.is_empty() {
+                            writeln!(
+                                f,
+                                "        Removing branches: {}",
+                                remove_branches.join(", ")
+                            )?;
+                        }
+                        if !add_tags.is_empty() {
+                            writeln!(f, "        Adding tags: {}", add_tags.join(", "))?;
+                        }
+                        if !remove_tags.is_empty() {
+                            writeln!(f, "        Removing tags: {}", remove_tags.join(", "))?;
+                        }
+                        if add_branches.is_empty()
+                            && remove_branches.is_empty()
+                            && add_tags.is_empty()
+                            && remove_tags.is_empty()
+                        {
+                            writeln!(f, "        No pattern changes")?;
+                        }
+                    }
                     EnvironmentDiff::Delete(name) => writeln!(f, "    ❌ Delete: {name}")?,
                 }
             }

sync-team/src/github/tests/mod.rs

@@ -1164,5 +1164,50 @@ fn repo_environment_update_branches() {
     );
 
     let diff = model.diff_repos(gh);
-    insta::assert_debug_snapshot!(diff, @"[]");
+    insta::assert_debug_snapshot!(diff, @r#"
+    [
+        Update(
+            UpdateRepoDiff {
+                org: "rust-lang",
+                name: "repo1",
+                repo_node_id: "0",
+                settings_diff: (
+                    RepoSettings {
+                        description: "",
+                        homepage: None,
+                        archived: false,
+                        auto_merge_enabled: false,
+                    },
+                    RepoSettings {
+                        description: "",
+                        homepage: None,
+                        archived: false,
+                        auto_merge_enabled: false,
+                    },
+                ),
+                permission_diffs: [],
+                branch_protection_diffs: [],
+                ruleset_diffs: [],
+                environment_diffs: [
+                    Update {
+                        name: "production",
+                        add_branches: [
+                            "stable",
+                        ],
+                        remove_branches: [
+                            "release/*",
+                        ],
+                        add_tags: [],
+                        remove_tags: [],
+                        new_branches: [
+                            "main",
+                            "stable",
+                        ],
+                        new_tags: [],
+                    },
+                ],
+            },
+        ),
+    ]
+    "#);
 }