refactor: address review comments

Author: amustaque97

src/schema.rs

@@ -3,7 +3,7 @@ pub(crate) use crate::permissions::Permissions;
 use anyhow::{bail, format_err, Error};
 use serde::de::{Deserialize, Deserializer};
 use serde_untagged::UntaggedEnumVisitor;
-use std::collections::{BTreeSet, HashMap, HashSet};
+use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
 
 #[derive(serde_derive::Deserialize, Debug)]
 #[serde(deny_unknown_fields, rename_all = "kebab-case")]
@@ -814,7 +814,7 @@ pub(crate) struct Repo {
     #[serde(default)]
     pub crates_io_publishing: Vec<CratesIoPublishing>,
     #[serde(default)]
-    pub environments: HashMap<String, Environment>,
+    pub environments: BTreeMap<String, Environment>,
 }
 
 #[derive(serde_derive::Deserialize, Debug, Clone, PartialEq)]

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

@@ -450,9 +450,16 @@ impl GithubRead for GitHubApiRead {
         org: &str,
         repo: &str,
     ) -> anyhow::Result<HashMap<String, Environment>> {
+        #[derive(serde::Deserialize)]
+        struct ProtectionRule {
+            #[serde(rename = "type")]
+            rule_type: String,
+        }
+
         #[derive(serde::Deserialize)]
         struct GitHubEnvironment {
             name: String,
+            protection_rules: Vec<ProtectionRule>,
         }
 
         #[derive(serde::Deserialize)]
@@ -470,50 +477,51 @@ impl GithubRead for GitHubApiRead {
             branch_policies: Vec<BranchPolicy>,
         }
 
-        let mut environment_names = Vec::new();
+        let mut env_infos = Vec::new();
 
-        // First, fetch all environment names
-        // REST API endpoint for environments
-        // https://docs.github.com/en/rest/deployments/environments#list-environments
+        // Fetch all environments with their protection_rules metadata
+        // REST API: https://docs.github.com/en/rest/deployments/environments#list-environments
         self.client.rest_paginated(
             &Method::GET,
             &GitHubUrl::repos(org, repo, "environments")?,
             |resp: EnvironmentsResponse| {
-                environment_names.extend(resp.environments.into_iter().map(|e| e.name));
+                env_infos.extend(resp.environments);
                 Ok(())
             },
         )?;
 
-        let mut environments: HashMap<String, Environment> = HashMap::new();
-
-        // Then, for each environment, fetch its deployment branch policies
-        // https://docs.github.com/en/rest/deployments/branch-policies#list-deployment-branch-policies
-        for env_name in environment_names {
-            let mut branches = Vec::new();
-
-            // Fetch branch policies for this environment
-            let result = self.client.rest_paginated(
-                &Method::GET,
-                &GitHubUrl::repos(
-                    org,
-                    repo,
-                    &format!("environments/{}/deployment-branch-policies", env_name),
-                )?,
-                |resp: BranchPoliciesResponse| {
-                    branches.extend(resp.branch_policies.into_iter().map(|p| p.name));
-                    Ok(())
-                },
-            );
-
-            // If the API call fails (e.g., 404 when no policies are configured),
-            // treat it as an empty branches list
-            if result.is_err() {
-                branches.clear();
-            }
-
-            environments.insert(env_name, Environment { branches });
-        }
-
-        Ok(environments)
+        // For each environment, fetch deployment branch policies if they exist
+        // REST API: https://docs.github.com/en/rest/deployments/branch-policies#list-deployment-branch-policies
+        env_infos
+            .into_iter()
+            .map(|env_info| {
+                // Check if branch policies exist by looking at protection_rules metadata
+                let has_branch_policies = env_info
+                    .protection_rules
+                    .iter()
+                    .any(|rule| rule.rule_type == "branch_policy");
+
+                let branches = if has_branch_policies {
+                    let mut branches = Vec::new();
+                    self.client.rest_paginated(
+                        &Method::GET,
+                        &GitHubUrl::repos(
+                            org,
+                            repo,
+                            &format!("environments/{}/deployment-branch-policies", env_info.name),
+                        )?,
+                        |resp: BranchPoliciesResponse| {
+                            branches.extend(resp.branch_policies.into_iter().map(|p| p.name));
+                            Ok(())
+                        },
+                    )?;
+                    branches
+                } else {
+                    Vec::new()
+                };
+
+                Ok((env_info.name, Environment { branches }))
+            })
+            .collect()
     }
 }

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

@@ -513,30 +513,7 @@ impl GitHubWrite {
             "Creating environment '{name}' in '{org}/{repo}' with branches: {:?}",
             branches
         );
-        if !self.dry_run {
-            // REST API: PUT /repos/{owner}/{repo}/environments/{environment_name}
-            // https://docs.github.com/en/rest/deployments/environments#create-or-update-an-environment
-            let url = GitHubUrl::repos(org, repo, &format!("environments/{}", name))?;
-
-            let body = if branches.is_empty() {
-                serde_json::json!({})
-            } else {
-                serde_json::json!({
-                    "deployment_branch_policy": {
-                        "protected_branches": false,
-                        "custom_branch_policies": true
-                    }
-                })
-            };
-
-            self.client.send(Method::PUT, &url, &body)?;
-
-            // If we have custom branches, we need to set them via a separate API call
-            if !branches.is_empty() {
-                self.set_environment_branch_policies(org, repo, name, branches)?;
-            }
-        }
-        Ok(())
+        self.upsert_environment(org, repo, name, branches)
     }
 
     /// Update an environment in a repository
@@ -551,6 +528,17 @@ impl GitHubWrite {
             "Updating environment '{name}' in '{org}/{repo}' with branches: {:?}",
             branches
         );
+        self.upsert_environment(org, repo, name, branches)
+    }
+
+    /// Internal helper to create or update an environment
+    fn upsert_environment(
+        &self,
+        org: &str,
+        repo: &str,
+        name: &str,
+        branches: &[String],
+    ) -> anyhow::Result<()> {
         if !self.dry_run {
             // REST API: PUT /repos/{owner}/{repo}/environments/{environment_name}
             // https://docs.github.com/en/rest/deployments/environments#create-or-update-an-environment
@@ -571,7 +559,7 @@ impl GitHubWrite {
 
             self.client.send(Method::PUT, &url, &body)?;
 
-            // This ensures old policies are cleaned up when transitioning to unrestricted deployments
+            // Always sync branch policies to ensure cleanup of old policies
             self.set_environment_branch_policies(org, repo, name, branches)?;
         }
         Ok(())