From d83581a3fc67830d3de723c745103d15eba5848b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= <sandro.jaeckel@sap.com>
Date: Fri, 30 Aug 2024 15:33:28 +0200
Subject: [PATCH] Implement Mocking for API client

---
 src/api_clients.rs | 326 ++++++++++++++++++++++++++++++++++++++++-----
 src/changes.rs     | 130 ++++++++++++------
 src/github.rs      |  12 +-
 src/main.rs        |  31 +++--
 src/remote.rs      | 146 +++++---------------
 5 files changed, 440 insertions(+), 205 deletions(-)

diff --git a/src/api_clients.rs b/src/api_clients.rs
index 91cb653..f375c33 100644
--- a/src/api_clients.rs
+++ b/src/api_clients.rs
@@ -14,75 +14,327 @@
 
 use std::collections::HashMap;
 use std::env;
-use std::sync::Arc;
+use std::future::Future;
+use std::sync::{Arc, Mutex};
 
-use anyhow::Context;
+use anyhow::{anyhow, Context};
+use octocrab::commits::PullRequestTarget;
+use octocrab::models::pulls::ReviewState;
+use octocrab::models::repos::RepoCommit;
 use octocrab::Octocrab;
-use tokio::sync::{AcquireError, Semaphore, SemaphorePermit};
+use tokio::sync::Semaphore;
 
+use crate::github::{Commit, PullRequest, Review};
 use crate::remote::Remote;
 
 #[derive(Debug)]
-pub struct Client {
+pub struct RealClient {
     semaphore: Semaphore,
     octocrab: Arc<Octocrab>,
 }
 
-impl Client {
-    pub async fn lock(&self) -> Result<(SemaphorePermit<'_>, &Arc<Octocrab>), AcquireError> {
-        let permit = self.semaphore.acquire().await?;
-        Ok((permit, &self.octocrab))
+pub trait Client {
+    fn new(env_name: String, api_endpoint: String) -> anyhow::Result<Arc<Self>>;
+
+    fn associated_prs(
+        &self,
+        owner: &str,
+        repo: &str,
+        sha: String,
+    ) -> impl Future<Output = anyhow::Result<Vec<PullRequest>>> + Send;
+
+    async fn compare(
+        &self,
+        owner: &str,
+        repo: &str,
+        original: &str,
+        base_commit: &str,
+        head_commit: &str,
+    ) -> anyhow::Result<Vec<Commit>>;
+
+    async fn pr_commits(&self, owner: &str, repo: &str, pr_number: u64) -> anyhow::Result<Vec<RepoCommit>>;
+
+    fn pr_head_hash(
+        &self,
+        owner: &str,
+        repo: &str,
+        pr_number: u64,
+    ) -> impl Future<Output = anyhow::Result<String>> + Send;
+
+    fn pr_reviews(
+        &self,
+        owner: &str,
+        repo: &str,
+        pr_number: u64,
+    ) -> impl Future<Output = anyhow::Result<Vec<Review>>> + Send;
+}
+
+impl Client for RealClient {
+    fn new(env_name: String, api_endpoint: String) -> anyhow::Result<Arc<RealClient>> {
+        octocrab::initialise(
+            Octocrab::builder()
+                .personal_token(env::var(&env_name).with_context(|| format!("missing {env_name} env"))?)
+                .base_uri(&api_endpoint)
+                .with_context(|| format!("failed to set base_uri to {api_endpoint}"))?
+                .build()
+                .context("failed to build octocrab client")?,
+        );
+        Ok(Arc::new(Self {
+            semaphore: Semaphore::new(5), // i.e. up to 5 API calls in parallel to the same GitHub instance
+            octocrab: octocrab::instance(),
+        }))
+    }
+
+    async fn associated_prs(&self, owner: &str, repo: &str, sha: String) -> anyhow::Result<Vec<PullRequest>> {
+        let _permit = self.semaphore.acquire().await?;
+
+        let mut associated_prs_page = self
+            .octocrab
+            .commits(owner, repo)
+            .associated_pull_requests(PullRequestTarget::Sha(sha))
+            .send()
+            .await
+            .context("failed to get associated prs")?;
+        assert!(
+            associated_prs_page.next.is_none(),
+            "found more than one page for associated_prs"
+        );
+
+        let associated_prs = associated_prs_page.take_items();
+
+        let mut prs: Vec<PullRequest> = Vec::new();
+        for associated_pr in associated_prs {
+            let associated_pr_url = associated_pr
+                .html_url
+                .as_ref()
+                .ok_or_else(|| anyhow!("pr without an html link!?"))?
+                .to_string();
+
+            prs.push(PullRequest {
+                number: associated_pr.number,
+                url: associated_pr_url,
+            });
+        }
+
+        Ok(prs)
+    }
+
+    async fn compare(
+        &self,
+        owner: &str,
+        repo: &str,
+        original: &str,
+        base_commit: &str,
+        head_commit: &str,
+    ) -> anyhow::Result<Vec<Commit>> {
+        let _permit = self.semaphore.acquire().await?;
+
+        let compare = self
+            .octocrab
+            .commits(owner, repo)
+            .compare(base_commit, head_commit)
+            .send()
+            .await
+            .context(format!(
+                "failed to compare {}/compare/{}...{}",
+                original.trim_end_matches(".git"),
+                &base_commit,
+                &head_commit
+            ))?;
+
+        let mut commits: Vec<Commit> = vec![];
+        for commit in compare.commits {
+            commits.push(Commit {
+                html_url: commit.html_url,
+                message: commit.commit.message,
+                sha: commit.sha,
+            });
+        }
+
+        Ok(commits)
+    }
+
+    async fn pr_head_hash(&self, owner: &str, repo: &str, pr_number: u64) -> Result<String, anyhow::Error> {
+        Ok(self
+            .pr_commits(owner, repo, pr_number)
+            .await
+            .context("failed to get pr commits")?
+            .last()
+            .ok_or_else(|| anyhow!("PR contains no commits?"))?
+            .sha
+            .clone())
+    }
+
+    async fn pr_commits(&self, owner: &str, repo: &str, pr_number: u64) -> anyhow::Result<Vec<RepoCommit>> {
+        let _permit = self.semaphore.acquire().await?;
+
+        let mut pr_commits_page = self
+            .octocrab
+            .pulls(owner, repo)
+            .pr_commits(pr_number)
+            .await
+            .context("failed to get pr commits")?;
+        assert!(
+            pr_commits_page.next.is_none(),
+            "found more than one page for associated_prs"
+        );
+
+        let pr_commits = pr_commits_page.take_items();
+        assert!(
+            pr_commits.len() <= 250,
+            "found more than 250 commits which requires a different api endpoint per doc"
+        );
+
+        Ok(pr_commits)
+    }
+
+    async fn pr_reviews(&self, owner: &str, repo: &str, pr_number: u64) -> anyhow::Result<Vec<Review>> {
+        let _permit = self.semaphore.acquire().await?;
+
+        let mut pr_reviews_page = self
+            .octocrab
+            .pulls(owner, repo)
+            .list_reviews(pr_number)
+            .send()
+            .await
+            .context("failed to get reviews")?;
+        assert!(
+            pr_reviews_page.next.is_none(),
+            "found more than one page for associated_prs"
+        );
+        let pr_reviews = pr_reviews_page.take_items();
+
+        let mut reviews = Vec::new();
+        for pr_review in &pr_reviews {
+            reviews.push(Review {
+                approved: pr_review.state == Some(ReviewState::Approved),
+                commit_id: pr_review.commit_id.clone().ok_or(anyhow!("review has no commit_id"))?,
+                submitted_at: pr_review
+                    .submitted_at
+                    .ok_or_else(|| anyhow!("review has no submitted_at"))?
+                    .timestamp_micros(),
+                user: pr_review.user.clone().ok_or(anyhow!("review has no user"))?.login,
+            });
+        }
+
+        reviews.sort_by_key(|r| r.submitted_at);
+        Ok(reviews)
     }
 }
 
-pub struct ClientSet {
-    clients: HashMap<String, Arc<Client>>,
+#[derive(Debug)]
+pub struct MockClient {
+    pub associated_prs: Mutex<HashMap<String, Vec<PullRequest>>>,
+    pub pr_commits: Mutex<HashMap<u64, Vec<RepoCommit>>>,
+    pub pr_head_hash: Mutex<HashMap<u64, String>>,
+    pub pr_reviews: Mutex<HashMap<u64, Vec<Review>>>,
+}
+
+impl Client for MockClient {
+    fn new(_env_name: String, _api_endpoint: String) -> anyhow::Result<Arc<Self>> {
+        Ok(Arc::new(Self {
+            associated_prs: Mutex::new(HashMap::new()),
+            pr_commits: Mutex::new(HashMap::new()),
+            pr_head_hash: Mutex::new(HashMap::new()),
+            pr_reviews: Mutex::new(HashMap::new()),
+        }))
+    }
+
+    async fn associated_prs(&self, _owner: &str, _repo: &str, sha: String) -> anyhow::Result<Vec<PullRequest>> {
+        Ok(self
+            .associated_prs
+            .lock()
+            .unwrap()
+            .get(&sha)
+            .ok_or_else(|| anyhow!("MockClient associated_prs contains no {}", sha))?
+            .clone())
+    }
+
+    async fn compare(
+        &self,
+        _owner: &str,
+        _repo: &str,
+        _original: &str,
+        _base_commit: &str,
+        _head_commit: &str,
+    ) -> anyhow::Result<Vec<Commit>> {
+        todo!()
+    }
+
+    async fn pr_head_hash(&self, _owner: &str, _repo: &str, pr_number: u64) -> anyhow::Result<String> {
+        Ok(self
+            .pr_head_hash
+            .lock()
+            .unwrap()
+            .get(&pr_number)
+            .ok_or_else(|| anyhow!("MockClient pr_head_hash contains no {}", pr_number))?
+            .to_string())
+    }
+
+    async fn pr_commits(&self, _owner: &str, _repo: &str, pr_number: u64) -> anyhow::Result<Vec<RepoCommit>> {
+        Ok(self
+            .pr_commits
+            .lock()
+            .unwrap()
+            .get(&pr_number)
+            .ok_or_else(|| anyhow!("MockClient pr_commits contains no {}", pr_number))?
+            .clone())
+    }
+
+    async fn pr_reviews(&self, _owner: &str, _repo: &str, pr_number: u64) -> anyhow::Result<Vec<Review>> {
+        Ok(self
+            .pr_reviews
+            .lock()
+            .unwrap()
+            .get(&pr_number)
+            .ok_or_else(|| anyhow!("MockClient pr_reviews contains no {}", pr_number))?
+            .clone())
+    }
 }
 
-impl ClientSet {
+pub struct ClientSet<C: Client> {
+    clients: HashMap<String, Arc<C>>,
+}
+
+impl<C: Client> ClientSet<C> {
     pub fn new() -> Self {
         Self {
             clients: HashMap::new(),
         }
     }
 
-    pub fn fill(&mut self, remote: &mut Remote) -> Result<(), anyhow::Error> {
+    pub fn fill(&mut self, remote: &mut Remote<C>) -> Result<(), anyhow::Error> {
         let host = remote.host.to_string();
         let client = self.get_client(&host)?;
         remote.client = Some(client);
         Ok(())
     }
 
-    // TODO: add test
-    fn get_client(&mut self, host: &str) -> Result<Arc<Client>, anyhow::Error> {
+    fn get_client(&mut self, host: &str) -> Result<Arc<C>, anyhow::Error> {
         if let Some(client) = self.clients.get(host) {
             return Ok(client.clone());
         }
 
-        let mut api_endpoint = "https://api.github.com".to_string();
-        let mut env_name = "GITHUB_TOKEN".to_string();
-
-        if host != "github.com" {
-            api_endpoint = format!("https://{host}/api/v3");
-            env_name = format!(
-                "GITHUB_{}_TOKEN",
-                host.replace('.', "_").to_uppercase().trim_start_matches("GITHUB_")
-            );
-        };
-
-        octocrab::initialise(
-            Octocrab::builder()
-                .personal_token(env::var(&env_name).with_context(|| format!("missing {env_name} env"))?)
-                .base_uri(&api_endpoint)
-                .with_context(|| format!("failed to set base_uri to {api_endpoint}"))?
-                .build()
-                .context("failed to build octocrab client")?,
-        );
-        let client = Arc::new(Client {
-            semaphore: Semaphore::new(5), // i.e. up to 5 API calls in parallel to the same GitHub instance
-            octocrab: octocrab::instance(),
-        });
+        let (env_name, api_endpoint) = get_env_name_api_endpoint_for_host(host);
+        let client = C::new(env_name, api_endpoint)?;
         self.clients.insert(host.to_owned(), client.clone());
+
         Ok(client)
     }
 }
+
+// TODO: add test
+fn get_env_name_api_endpoint_for_host(host: &str) -> (String, String) {
+    let mut env_name = "GITHUB_TOKEN".to_string();
+    let mut api_endpoint = "https://api.github.com".to_string();
+
+    if host != "github.com" {
+        api_endpoint = format!("https://{host}/api/v3");
+        env_name = format!(
+            "GITHUB_{}_TOKEN",
+            host.replace('.', "_").to_uppercase().trim_start_matches("GITHUB_")
+        );
+    };
+
+    (env_name, api_endpoint)
+}
diff --git a/src/changes.rs b/src/changes.rs
index 5742bb4..f15ff0c 100644
--- a/src/changes.rs
+++ b/src/changes.rs
@@ -12,88 +12,95 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use anyhow::{anyhow, Context};
+use std::sync::Arc;
+
+use anyhow::Context;
 use tokio::task::JoinSet;
 
+use crate::api_clients::Client;
 use crate::github::{Commit, Review};
 use crate::remote::Remote;
 
-#[derive(Clone, Debug)]
-pub struct RepoChangeset {
+#[derive(Debug)]
+pub struct RepoChangeset<C: Client> {
     pub name: String,
-    pub remote: Remote,
+    pub remote: Remote<C>,
     pub base_commit: String,
     pub head_commit: String,
     pub changes: Vec<Changeset>,
 }
 
-impl RepoChangeset {
-    pub async fn analyze_commits(mut self) -> Result<Self, anyhow::Error> {
+impl<C: Client + Sync + Send + 'static> RepoChangeset<C> {
+    pub async fn analyze_commits(mut self) -> anyhow::Result<Self> {
         let compare_commits = self.remote.compare(&self.base_commit, &self.head_commit).await?;
 
         let mut join_set = JoinSet::new();
+        let remote = Arc::new(self.remote);
         for commit in compare_commits {
-            join_set.spawn(self.clone().analyze_commit(commit));
+            join_set.spawn(Self::analyze_commit(remote.clone(), commit));
         }
 
+        let mut changesets: Vec<Changeset> = vec![];
         while let Some(res) = join_set.join_next().await {
             let changes = res?.context("while collecting change")?;
-            for change in changes {
-                self.changes.push(change);
+            for change in &changes {
+                changesets.push(change.clone());
+            }
+        }
+
+        for change in &changesets {
+            if let Some(self_change) = self
+                .changes
+                .iter_mut()
+                .find(|self_change| self_change.pr_link == change.pr_link)
+            {
+                for approval in &change.approvals {
+                    self_change.approvals.push(approval.clone());
+                }
+                continue;
             }
+
+            self.changes.push(change.clone());
         }
 
+        self.remote = Arc::into_inner(remote).unwrap();
         Ok(self)
     }
 
     // TODO: add test
-    async fn analyze_commit(mut self, commit: Commit) -> Result<Vec<Changeset>, anyhow::Error> {
+    async fn analyze_commit(remote: Arc<Remote<C>>, commit: Commit) -> anyhow::Result<Vec<Changeset>> {
         let change_commit = CommitMetadata::new(&commit);
+        let mut changes = vec![];
 
-        let associated_prs = self.remote.associated_prs(commit.sha.clone()).await?;
+        let associated_prs = remote.associated_prs(commit.sha.clone()).await?;
         if associated_prs.is_empty() {
-            self.changes.push(Changeset {
+            changes.push(Changeset {
                 commits: vec![change_commit],
                 pr_link: None,
                 approvals: Vec::new(),
             });
-            return Ok(self.changes);
+            return Ok(changes);
         }
 
         for associated_pr in &associated_prs {
-            let pr_reviews = self.remote.pr_reviews(associated_pr.number).await?;
-
-            let associated_pr_link = Some(
-                associated_pr
-                    .html_url
-                    .as_ref()
-                    .ok_or_else(|| anyhow!("pr without an html link!?"))?
-                    .to_string(),
-            );
-
-            let head_sha = self.remote.pr_head_hash(associated_pr.number).await?;
-
-            if let Some(changeset) = self.changes.iter_mut().find(|cs| cs.pr_link == associated_pr_link) {
-                changeset.commits.push(change_commit.clone());
-                changeset.collect_approved_reviews(&pr_reviews, &head_sha);
-                continue;
-            }
-
             let mut changeset = Changeset {
                 commits: vec![change_commit.clone()],
-                pr_link: associated_pr_link,
+                pr_link: Some(associated_pr.url.clone()),
                 approvals: Vec::new(),
             };
 
+            let pr_reviews = remote.pr_reviews(associated_pr.number).await?;
+            let head_sha = remote.pr_head_hash(associated_pr.number).await?;
             changeset.collect_approved_reviews(&pr_reviews, &head_sha);
-            self.changes.push(changeset);
+
+            changes.push(changeset);
         }
 
-        Ok(self.changes)
+        Ok(changes)
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq)]
 pub struct Changeset {
     pub commits: Vec<CommitMetadata>,
     pub pr_link: Option<String>,
@@ -102,7 +109,6 @@ pub struct Changeset {
 
 impl Changeset {
     // pr_reviews must be sorted by key submitted_at!
-    // TODO: add test
     pub fn collect_approved_reviews(&mut self, pr_reviews: &[Review], head_sha: &String) {
         let mut last_review_by: Vec<String> = vec![];
 
@@ -135,7 +141,7 @@ impl Changeset {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq)]
 pub struct CommitMetadata {
     pub headline: String,
     pub link: String,
@@ -159,7 +165,8 @@ impl CommitMetadata {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::github::Review;
+    use crate::api_clients::{ClientSet, MockClient};
+    use crate::github::{PullRequest, Review};
 
     fn gen_change_review() -> (Changeset, Vec<Review>) {
         (
@@ -213,4 +220,51 @@ mod tests {
         changeset.collect_approved_reviews(&pr_reviews, &"00000000000000000000000000000003".to_owned());
         assert_eq!(changeset.approvals, Vec::<String>::new());
     }
+
+    #[tokio::test]
+    async fn analyze_commit() {
+        let mut api_clients = ClientSet::new();
+        let mut remote = Remote::<MockClient>::parse("https://github.com/example/project.git").unwrap();
+        api_clients.fill(&mut remote).unwrap();
+
+        let remote_client = (&mut remote.client)
+            .as_ref()
+            .unwrap();
+
+        remote_client.associated_prs
+            .lock()
+            .unwrap()
+            .insert("00000000000000000000000000000002".to_string(), vec![PullRequest {
+                number: 1,
+                url: "https://github.com/example/project/pull/1".to_owned(),
+            }]);
+
+
+            remote_client.pr_reviews
+            .lock()
+            .unwrap()
+            .insert(1, vec![Review {
+              approved: 1
+              commit_id: "00000000000000000000000000000002".to_owned(),
+              submitted_at: 42,
+              user: "Example",
+            }]);
+
+        let changeset = RepoChangeset::analyze_commit(remote.into(), Commit {
+            html_url: "https://github.com/example/project/commit/00000000000000000000000000000002".to_owned(),
+            message: "Testing test".to_owned(),
+            sha: "00000000000000000000000000000002".to_owned(),
+        })
+        .await
+        .unwrap();
+
+        assert_eq!(changeset[0], Changeset {
+            approvals: vec!["user1".to_owned()],
+            commits: vec![CommitMetadata {
+                headline: "Testing test".to_owned(),
+                link: "https://github.com/example/project/commit/00000000000000000000000000000002".to_owned(),
+            }],
+            pr_link: Some("https://github.com/example/project/pulls/1".to_owned()),
+        });
+    }
 }
diff --git a/src/github.rs b/src/github.rs
index 6d38c3a..8f6554e 100644
--- a/src/github.rs
+++ b/src/github.rs
@@ -14,9 +14,15 @@
 
 #[derive(Clone, Debug)]
 pub struct Commit {
-  pub html_url: String,
-  pub message: String,
-  pub sha: String,
+    pub html_url: String,
+    pub message: String,
+    pub sha: String,
+}
+
+#[derive(Clone, Debug)]
+pub struct PullRequest {
+    pub number: u64,
+    pub url: String,
 }
 
 #[derive(Clone, Debug)]
diff --git a/src/main.rs b/src/main.rs
index d32b8a4..fd0e7e9 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -25,7 +25,7 @@ use std::str;
 use std::sync::LazyLock;
 
 use anyhow::{anyhow, Context};
-use api_clients::ClientSet;
+use api_clients::{ClientSet, RealClient};
 use changes::RepoChangeset;
 use clap::builder::styling::Style;
 use clap::{Parser, Subcommand};
@@ -135,7 +135,11 @@ async fn main() -> Result<(), anyhow::Error> {
     Ok(())
 }
 
-fn find_values_yaml(workspace: String, base: &str, head: &str) -> Result<Vec<RepoChangeset>, anyhow::Error> {
+fn find_values_yaml(
+    workspace: String,
+    base: &str,
+    head: &str,
+) -> Result<Vec<RepoChangeset<RealClient>>, anyhow::Error> {
     let repo = Repository::open(workspace).context("failed to open repository")?;
 
     let base_tree = repo::tree_for_commit_ref(&repo, base)?;
@@ -144,7 +148,7 @@ fn find_values_yaml(workspace: String, base: &str, head: &str) -> Result<Vec<Rep
         .diff_tree_to_tree(Some(&head_tree), Some(&base_tree), None)
         .with_context(|| format!("cannot diff trees {} and {}", base_tree.id(), head_tree.id()))?;
 
-    let mut changes = Vec::<RepoChangeset>::new();
+    let mut changes = Vec::<RepoChangeset<RealClient>>::new();
 
     for diff_delta in diff_tree.deltas() {
         let new_file = diff_delta.new_file();
@@ -166,14 +170,15 @@ fn find_values_yaml(workspace: String, base: &str, head: &str) -> Result<Vec<Rep
         let old_image_refs = ImageRefs::parse(&repo, &old_file)?;
         for (name, image) in &new_image_refs.container_images {
             for source in &image.sources {
-                changes.push(RepoChangeset {
-                    name: name.clone(),
-                    remote: remote::Remote::parse(&source.repo)?,
-                    // TODO: iterate over sources
-                    base_commit: source.commit.clone(),
-                    head_commit: old_image_refs.container_images[name].sources[0].commit.clone(),
-                    changes: Vec::new(),
-                });
+                for container_image_source in &old_image_refs.container_images[name].sources {
+                    changes.push(RepoChangeset {
+                        name: name.clone(),
+                        remote: remote::Remote::parse(&source.repo)?,
+                        base_commit: source.commit.clone(),
+                        head_commit: container_image_source.commit.clone(),
+                        changes: Vec::new(),
+                    });
+                }
             }
         }
     }
@@ -181,8 +186,8 @@ fn find_values_yaml(workspace: String, base: &str, head: &str) -> Result<Vec<Rep
     Ok(changes)
 }
 
-fn print_changes(changes: &[RepoChangeset]) -> Result<(), anyhow::Error> {
-    for change in changes {
+fn print_changes(repo_changeset: &[RepoChangeset<RealClient>]) -> Result<(), anyhow::Error> {
+    for change in repo_changeset {
         println!(
             "Name {} from {} moved from {} to {}",
             change.name, change.remote.original, change.base_commit, change.head_commit
diff --git a/src/remote.rs b/src/remote.rs
index 2f188d1..7c0c30c 100644
--- a/src/remote.rs
+++ b/src/remote.rs
@@ -15,29 +15,23 @@
 use std::sync::Arc;
 
 use anyhow::{anyhow, bail, Context};
-use octocrab::commits::PullRequestTarget;
-use octocrab::models::pulls::{PullRequest, ReviewState};
-use octocrab::models::repos::RepoCommit;
-use octocrab::Octocrab;
-use tokio::sync::SemaphorePermit;
 use url::Url;
 
 use crate::api_clients::Client;
-use crate::github::Commit;
-use crate::github::Review;
+use crate::github::{Commit, PullRequest, Review};
 
 #[derive(Clone, Debug)]
 #[allow(dead_code)]
-pub struct Remote {
+pub struct Remote<C: Client> {
     pub host: url::Host,
     pub port: u16,
     pub owner: String,
     pub repository: String,
     pub original: String,
-    pub client: Option<Arc<Client>>,
+    pub client: Option<Arc<C>>,
 }
 
-impl Remote {
+impl<C: Client> Remote<C> {
     pub fn parse(url: &str) -> Result<Self, anyhow::Error> {
         let remote_url = Url::parse(url).context("can't parse remote")?;
         let path_elements: Vec<&str> = remote_url.path().trim_start_matches('/').split('/').collect();
@@ -56,131 +50,55 @@ impl Remote {
         })
     }
 
-    async fn get_client(&self) -> Result<(SemaphorePermit<'_>, &Arc<Octocrab>), anyhow::Error> {
-        let client = self
-            .client
+    pub async fn associated_prs(&self, sha: String) -> anyhow::Result<Vec<PullRequest>> {
+        self.client
             .as_ref()
-            .ok_or_else(|| anyhow!("no client attached to remote"))?;
-        client.lock().await.context("cannot obtain semaphore for client")
-    }
-
-    pub async fn associated_prs(&self, sha: String) -> Result<Vec<PullRequest>, anyhow::Error> {
-        let (_permit, octocrab) = self.get_client().await?;
-
-        let mut associated_prs_page = octocrab
-            .commits(&self.owner, &self.repository)
-            .associated_pull_requests(PullRequestTarget::Sha(sha))
-            .send()
+            .ok_or_else(|| anyhow!("no client attached to remote"))?
+            .associated_prs(&self.owner, &self.repository, sha)
             .await
-            .context("failed to get associated prs")?;
-        assert!(
-            associated_prs_page.next.is_none(),
-            "found more than one page for associated_prs"
-        );
-        Ok(associated_prs_page.take_items())
     }
 
-    pub async fn compare(&self, base_commit: &str, head_commit: &str) -> Result<Vec<Commit>, anyhow::Error> {
-        let (_permit, octocrab) = self.get_client().await?;
-
-        let compare = octocrab
-            .commits(&self.owner, &self.repository)
-            .compare(base_commit, head_commit)
-            .send()
-            .await
-            .context(format!(
-                "failed to compare {}/compare/{}...{}",
-                self.original.trim_end_matches(".git"),
-                &base_commit,
-                &head_commit
-            ))?;
-
-        let mut commits: Vec<Commit> = vec![];
-        for commit in compare.commits {
-            commits.push(Commit {
-              html_url: commit.html_url,
-              message: commit.commit.message,
-              sha: commit.sha,
-            });
-        }
-
-        Ok(commits)
-    }
-
-    pub async fn pr_head_hash(&self, pr_number: u64) -> Result<String, anyhow::Error> {
-        Ok(self
-            .pr_commits(pr_number)
+    pub async fn compare(&self, base_commit: &str, head_commit: &str) -> anyhow::Result<Vec<Commit>> {
+        self.client
+            .as_ref()
+            .ok_or_else(|| anyhow!("no client attached to remote"))?
+            .compare(
+                &self.owner,
+                &self.repository,
+                &self.original,
+                base_commit,
+                head_commit,
+            )
             .await
-            .context("failed to get pr commits")?
-            .last()
-            .ok_or_else(|| anyhow!("PR contains no commits?"))?
-            .sha
-            .clone())
     }
 
-    pub async fn pr_commits(&self, pr_number: u64) -> Result<Vec<RepoCommit>, anyhow::Error> {
-        let (_permit, octocrab) = self.get_client().await?;
-
-        let mut pr_commits_page = octocrab
-            .pulls(&self.owner, &self.repository)
-            .pr_commits(pr_number)
+    pub async fn pr_head_hash(&self,  pr_number: u64) -> Result<String, anyhow::Error> {
+        self.client
+            .as_ref()
+            .ok_or_else(|| anyhow!("no client attached to remote"))?
+            .pr_head_hash(&self.owner, &self.repository, pr_number)
             .await
-            .context("failed to get pr commits")?;
-        assert!(
-            pr_commits_page.next.is_none(),
-            "found more than one page for associated_prs"
-        );
-
-        let pr_commits = pr_commits_page.take_items();
-        assert!(
-            pr_commits.len() <= 250,
-            "found more than 250 commits which requires a different api endpoint per doc"
-        );
-
-        Ok(pr_commits)
     }
 
     pub async fn pr_reviews(&self, pr_number: u64) -> Result<Vec<Review>, anyhow::Error> {
-        let (_permit, octocrab) = self.get_client().await?;
-
-        let mut pr_reviews_page = octocrab
-            .pulls(&self.owner, &self.repository)
-            .list_reviews(pr_number)
-            .send()
+        self.client
+            .as_ref()
+            .ok_or_else(|| anyhow!("no client attached to remote"))?
+            .pr_reviews(&self.owner, &self.repository, pr_number)
             .await
-            .context("failed to get reviews")?;
-        assert!(
-            pr_reviews_page.next.is_none(),
-            "found more than one page for associated_prs"
-        );
-        let pr_reviews = pr_reviews_page.take_items();
-
-        let mut reviews = Vec::new();
-        for pr_review in &pr_reviews {
-            reviews.push(Review {
-                approved: pr_review.state == Some(ReviewState::Approved),
-                commit_id: pr_review.commit_id.clone().ok_or(anyhow!("review has no commit_id"))?,
-                submitted_at: pr_review
-                    .submitted_at
-                    .ok_or_else(|| anyhow!("review has no submitted_at"))?
-                    .timestamp_micros(),
-                user: pr_review.user.clone().ok_or(anyhow!("review has no user"))?.login,
-            });
-        }
-
-        reviews.sort_by_key(|r| r.submitted_at);
-        Ok(reviews)
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use crate::api_clients::RealClient;
+
     use super::*;
 
     #[test]
     fn parse_remote() -> Result<(), anyhow::Error> {
         let remote = "https://github.com/sapcc/pear-reviewer.git";
-        let result = Remote::parse(remote)?;
+        let result = Remote::<RealClient>::parse(remote)?;
         assert_eq!(result.host, url::Host::Domain("github.com"));
         assert_eq!(result.owner, "sapcc");
         assert_eq!(result.repository, "pear-reviewer");
@@ -190,7 +108,7 @@ mod tests {
 
     #[test]
     fn parse_remote_invalid() {
-        let result = Remote::parse("https://sapcc/pear-reviewer.git");
+        let result = Remote::<RealClient>::parse("https://sapcc/pear-reviewer.git");
         match result {
             Err(err) => {
                 assert_eq!(