fix fetching environment branch policies

Author: marcoieni

Cargo.lock

@@ -68,5 +68,6 @@ serde_json = "1.0"
 serde-untagged = "0.1"
 sync-team = { path = "sync-team" }
 tempfile = "3.19.1"
+thiserror = "2.0.18"
 toml = "1.0"
 walkdir = "2.3.1"

Cargo.toml

@@ -14,6 +14,7 @@ hyper-old-types.workspace = true
 serde_json.workspace = true
 secrecy.workspace = true
 indexmap.workspace = true
+thiserror.workspace = true
 
 [dev-dependencies]
 indexmap.workspace = true

sync-team/Cargo.toml

@@ -18,12 +18,27 @@ use reqwest::{
 use secrecy::ExposeSecret;
 use serde::{Deserialize, de::DeserializeOwned};
 use std::fmt;
+use thiserror::Error;
 use tokens::GitHubTokens;
 use url::GitHubUrl;
 
 pub(crate) use read::{GitHubApiRead, GithubRead};
 pub(crate) use write::GitHubWrite;
 
+#[derive(Debug, Error)]
+pub(crate) enum RestPaginatedError {
+    #[error("{method} request to '{url}' failed with status {status}")]
+    Http {
+        method: Method,
+        url: String,
+        status: StatusCode,
+        #[source]
+        source: anyhow::Error,
+    },
+    #[error(transparent)]
+    Other(#[from] anyhow::Error),
+}
+
 #[derive(Clone)]
 pub(crate) struct HttpClient {
     client: Client,
@@ -157,7 +172,12 @@ impl HttpClient {
         })
     }
 
-    fn rest_paginated<F, T>(&self, method: &Method, url: &GitHubUrl, mut f: F) -> anyhow::Result<()>
+    fn rest_paginated<F, T>(
+        &self,
+        method: &Method,
+        url: &GitHubUrl,
+        mut f: F,
+    ) -> Result<(), RestPaginatedError>
     where
         F: FnMut(T) -> anyhow::Result<()>,
         T: DeserializeOwned,
@@ -167,12 +187,25 @@ impl HttpClient {
             let resp = self
                 .req(method.clone(), &next_url)?
                 .send()
-                .with_context(|| format!("failed to send request to {}", next_url.url()))?
-                .custom_error_for_status()?;
+                .with_context(|| format!("failed to send request to {}", next_url.url()))?;
+
+            let status = resp.status();
+            let resp =
+                resp.custom_error_for_status()
+                    .map_err(|source| RestPaginatedError::Http {
+                        method: method.clone(),
+                        url: next_url.url().to_string(),
+                        status,
+                        source,
+                    })?;
 
             // Extract the next page
             if let Some(links) = resp.headers().get(header::LINK) {
-                let links: Link = links.to_str()?.parse()?;
+                let links: Link = links
+                    .to_str()
+                    .context("failed to convert LINK header to string")?
+                    .parse()
+                    .context("failed to parse LINK header")?;
                 for link in links.values() {
                     if link
                         .rel()
@@ -637,3 +670,15 @@ pub(crate) enum RulesetOp {
     CreateForRepo,
     UpdateRuleset(i64),
 }
+
+#[derive(Debug, serde::Deserialize)]
+pub(crate) struct BranchPolicy {
+    pub(crate) id: u64,
+    pub(crate) name: String,
+    #[serde(rename = "type", default = "default_branch_policy_type")]
+    pattern_type: String,
+}
+
+fn default_branch_policy_type() -> String {
+    "branch".to_string()
+}

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

@@ -1,10 +1,11 @@
-use crate::github::api::Ruleset;
+use crate::github::api::{BranchPolicy, 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,
+    RepoUser, RestPaginatedError, Team, TeamMember, TeamRole, team_node_id, url::GitHubUrl,
+    user_node_id,
 };
 use anyhow::Context as _;
-use reqwest::Method;
+use reqwest::{Method, StatusCode};
 use rust_team_data::v1::Environment;
 use std::collections::{HashMap, HashSet};
 
@@ -68,6 +69,13 @@ pub(crate) trait GithubRead {
         org: &str,
         repo: &str,
     ) -> anyhow::Result<Vec<crate::github::api::Ruleset>>;
+
+    fn environment_branch_policies(
+        &self,
+        org: &str,
+        repo: &str,
+        environment: &str,
+    ) -> anyhow::Result<Vec<BranchPolicy>>;
 }
 
 pub(crate) struct GitHubApiRead {
@@ -479,22 +487,6 @@ impl GithubRead for GitHubApiRead {
             environments: Vec<GitHubEnvironment>,
         }
 
-        #[derive(serde::Deserialize)]
-        struct BranchPolicy {
-            name: String,
-            #[serde(rename = "type", default = "default_branch_type")]
-            pattern_type: String,
-        }
-
-        fn default_branch_type() -> String {
-            "branch".to_string()
-        }
-
-        #[derive(serde::Deserialize)]
-        struct BranchPoliciesResponse {
-            branch_policies: Vec<BranchPolicy>,
-        }
-
         let mut env_infos = Vec::new();
 
         // Fetch all environments with their protection_rules metadata
@@ -522,23 +514,21 @@ impl GithubRead for GitHubApiRead {
                 let (branches, tags) = if has_branch_policies {
                     let mut branches = Vec::new();
                     let mut tags = Vec::new();
-                    self.client.rest_paginated(
-                        &Method::GET,
-                        &GitHubUrl::repos(
-                            org,
-                            repo,
-                            &format!("environments/{}/deployment-branch-policies", env_info.name),
-                        )?,
-                        |resp: BranchPoliciesResponse| {
-                            for p in resp.branch_policies {
-                                match p.pattern_type.as_str() {
-                                    "tag" => tags.push(p.name),
-                                    _ => branches.push(p.name),
-                                }
-                            }
-                            Ok(())
-                        },
-                    )?;
+                    let policies =
+                        self.environment_branch_policies(org, repo, &env_info.name).with_context(
+                            || {
+                                format!(
+                                    "failed to load deployment branch policies for environment '{}' in '{org}/{repo}'",
+                                    env_info.name
+                                )
+                            },
+                        )?;
+                    for p in policies {
+                        match p.pattern_type.as_str() {
+                            "tag" => tags.push(p.name),
+                            _ => branches.push(p.name),
+                        }
+                    }
                     (branches, tags)
                 } else {
                     (Vec::new(), Vec::new())
@@ -570,4 +560,47 @@ impl GithubRead for GitHubApiRead {
 
         Ok(rulesets)
     }
+
+    fn environment_branch_policies(
+        &self,
+        org: &str,
+        repo: &str,
+        environment: &str,
+    ) -> anyhow::Result<Vec<BranchPolicy>> {
+        #[derive(serde::Deserialize)]
+        struct BranchPoliciesResponse {
+            branch_policies: Vec<BranchPolicy>,
+        }
+
+        let mut policies = Vec::new();
+        let url = GitHubUrl::repos(
+            org,
+            repo,
+            &format!("environments/{environment}/deployment-branch-policies"),
+        )?;
+
+        if let Err(err) =
+            self.client
+                .rest_paginated(&Method::GET, &url, |resp: BranchPoliciesResponse| {
+                    policies.extend(resp.branch_policies);
+                    Ok(())
+                })
+        {
+            match err {
+                RestPaginatedError::Http {
+                    status: StatusCode::NOT_FOUND,
+                    ..
+                } => return Ok(policies),
+                other => {
+                    return Err(anyhow::Error::from(other)).with_context(|| {
+                        format!(
+                            "failed to fetch deployment branch policies for environment '{environment}' in '{org}/{repo}'"
+                        )
+                    });
+                }
+            }
+        }
+
+        Ok(policies)
+    }
 }

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

@@ -4,19 +4,12 @@ use std::collections::HashSet;
 
 use crate::github::api::url::GitHubUrl;
 use crate::github::api::{
-    AppPushAllowanceActor, BranchProtection, BranchProtectionOp, HttpClient, Login,
-    PushAllowanceActor, Repo, RepoPermission, RepoSettings, Team, TeamPrivacy,
+    AppPushAllowanceActor, BranchProtection, BranchProtectionOp, GitHubApiRead, GithubRead,
+    HttpClient, Login, PushAllowanceActor, Repo, RepoPermission, RepoSettings, Team, TeamPrivacy,
     TeamPushAllowanceActor, TeamRole, UserPushAllowanceActor, allow_not_found,
 };
 use crate::utils::ResponseExt;
 
-#[derive(Debug)]
-struct BranchPolicyInfo {
-    id: u64,
-    name: String,
-    pattern_type: String,
-}
-
 pub(crate) struct GitHubWrite {
     client: HttpClient,
     dry_run: bool,
@@ -570,50 +563,6 @@ impl GitHubWrite {
         Ok(())
     }
 
-    /// Get existing branch policies for an environment
-    fn get_environment_branch_policies(
-        &self,
-        org: &str,
-        repo: &str,
-        environment: &str,
-    ) -> anyhow::Result<Vec<BranchPolicyInfo>> {
-        #[derive(serde::Deserialize)]
-        struct BranchPolicy {
-            id: u64,
-            name: String,
-            #[serde(rename = "type", default = "default_branch_type")]
-            pattern_type: String,
-        }
-
-        fn default_branch_type() -> String {
-            "branch".to_string()
-        }
-
-        #[derive(serde::Deserialize)]
-        struct BranchPoliciesResponse {
-            branch_policies: Vec<BranchPolicy>,
-        }
-
-        let url = GitHubUrl::repos(
-            org,
-            repo,
-            &format!("environments/{}/deployment-branch-policies", environment),
-        )?;
-
-        let response: BranchPoliciesResponse =
-            self.client.req(Method::GET, &url)?.send()?.json()?;
-
-        Ok(response
-            .branch_policies
-            .into_iter()
-            .map(|bp| BranchPolicyInfo {
-                id: bp.id,
-                name: bp.name,
-                pattern_type: bp.pattern_type,
-            })
-            .collect())
-    }
-
     /// Delete a specific branch policy by ID
     fn delete_environment_branch_policy(
         &self,
@@ -649,7 +598,8 @@ impl GitHubWrite {
         tags: &[String],
     ) -> anyhow::Result<()> {
         // 1. Fetch existing policies
-        let existing_policies = self.get_environment_branch_policies(org, repo, environment)?;
+        let existing_policies = GitHubApiRead::from_client(self.client.clone())?
+            .environment_branch_policies(org, repo, environment)?;
 
         #[derive(Hash, Eq, PartialEq)]
         struct PatternKey {

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

@@ -10,7 +10,8 @@ use rust_team_data::v1::{
 
 use crate::Config;
 use crate::github::api::{
-    BranchProtection, GithubRead, Repo, RepoTeam, RepoUser, Team, TeamMember, TeamPrivacy, TeamRole,
+    BranchPolicy, BranchProtection, GithubRead, Repo, RepoTeam, RepoUser, Team, TeamMember,
+    TeamPrivacy, TeamRole,
 };
 use crate::github::{
     OrgMembershipDiff, RepoDiff, SyncGitHub, TeamDiff, api, construct_branch_protection,
@@ -648,6 +649,17 @@ impl GithubRead for GithubMock {
             .cloned()
             .unwrap_or_default())
     }
+
+    fn environment_branch_policies(
+        &self,
+        _org: &str,
+        _repo: &str,
+        _environment: &str,
+    ) -> anyhow::Result<Vec<BranchPolicy>> {
+        unimplemented!(
+            "call the function repo_environments instead, and read branch policies from there"
+        )
+    }
 }
 
 #[derive(Default)]