eval: use traits github interactions

ofborg/src/ghgist.rs

@@ -1,15 +1,21 @@
 use hubcaps::gists::{Gist, GistOptions};
 
-pub struct Client<'a> {
+pub trait Client {
+    fn create_gist(&self, gist: &GistOptions) -> hubcaps::Result<Gist>;
+}
+
+pub struct Hubcaps<'a> {
     github: &'a hubcaps::Github,
 }
 
-impl<'a> Client<'a> {
+impl<'a> Hubcaps<'a> {
     pub fn new(github: &'a hubcaps::Github) -> Self {
-        Client { github }
+        Hubcaps { github }
     }
+}
 
-    pub fn create_gist(&self, gist: &GistOptions) -> hubcaps::Result<Gist> {
+impl Client for Hubcaps<'_> {
+    fn create_gist(&self, gist: &GistOptions) -> hubcaps::Result<Gist> {
         self.github.gists().create(gist)
     }
 }

ofborg/src/ghrepo.rs

@@ -10,34 +10,57 @@ use hubcaps::review_requests::ReviewRequestOptions;
 use hubcaps::statuses::{Status, StatusOptions};
 use hubcaps::Github;
 
-pub struct Client<'a> {
+pub trait Client {
+    fn get_issue(&self, number: u64) -> hubcaps::Result<Issue>;
+    fn get_pull(&self, number: u64) -> hubcaps::Result<Pull>;
+    fn add_labels(&self, number: u64, labels: Vec<&str>) -> hubcaps::Result<Vec<Label>>;
+    fn remove_label(&self, number: u64, label: &str) -> hubcaps::Result<()>;
+    fn create_review_request(
+        &self,
+        number: u64,
+        review_request: &ReviewRequestOptions,
+    ) -> hubcaps::Result<Pull>;
+    fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result<Status>;
+    fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result<CheckRun>;
+    fn create_commitstatus(
+        &self,
+        pr: &message::Pr,
+        context: String,
+        description: String,
+        gist_url: Option<String>,
+    ) -> commitstatus::CommitStatus;
+}
+
+pub struct Hubcaps<'a> {
     repo: Repository<'a>,
 }
 
-impl<'a> Client<'a> {
+impl<'a> Hubcaps<'a> {
     pub fn new(github: &'a Github, repo: &message::Repo) -> Self {
         let repo = github.repo(repo.owner.clone(), repo.name.clone());
-        Client { repo }
+        Hubcaps { repo }
     }
+}
 
-    pub fn get_issue(&self, number: u64) -> hubcaps::Result<Issue> {
+impl Client for Hubcaps<'_> {
+    fn get_issue(&self, number: u64) -> hubcaps::Result<Issue> {
         self.repo.issue(number).get()
     }
 
-    pub fn get_pull(&self, number: u64) -> hubcaps::Result<Pull> {
+    fn get_pull(&self, number: u64) -> hubcaps::Result<Pull> {
         let pulls = self.repo.pulls();
         pulls.get(number).get()
     }
 
-    pub fn add_labels(&self, number: u64, labels: Vec<&str>) -> hubcaps::Result<Vec<Label>> {
+    fn add_labels(&self, number: u64, labels: Vec<&str>) -> hubcaps::Result<Vec<Label>> {
         self.repo.issue(number).labels().add(labels)
     }
 
-    pub fn remove_label(&self, number: u64, label: &str) -> hubcaps::Result<()> {
+    fn remove_label(&self, number: u64, label: &str) -> hubcaps::Result<()> {
         self.repo.issue(number).labels().remove(label)
     }
 
-    pub fn create_review_request(
+    fn create_review_request(
         &self,
         number: u64,
         review_request: &ReviewRequestOptions,
@@ -46,15 +69,15 @@ impl<'a> Client<'a> {
         pulls.get(number).review_requests().create(review_request)
     }
 
-    pub fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result<Status> {
+    fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result<Status> {
         self.repo.statuses().create(sha, status)
     }
 
-    pub fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result<CheckRun> {
+    fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result<CheckRun> {
         self.repo.checkruns().create(&check)
     }
 
-    pub fn create_commitstatus(
+    fn create_commitstatus(
         &self,
         pr: &message::Pr,
         context: String,

ofborg/src/tasks/eval/nixpkgs.rs

@@ -21,6 +21,7 @@ use crate::tasks::evaluate::{make_gist, update_labels};
 
 use std::collections::HashMap;
 use std::path::Path;
+use std::rc::Rc;
 
 use chrono::Utc;
 use hubcaps::checks::{CheckRunOptions, CheckRunState, Conclusion, Output};
@@ -30,8 +31,8 @@ use uuid::Uuid;
 static MAINTAINER_REVIEW_MAX_CHANGED_PATHS: usize = 64;
 
 pub struct NixpkgsStrategy<'a> {
-    repo_client: &'a ghrepo::Client<'a>,
-    gist_client: &'a ghgist::Client<'a>,
+    repo_client: Rc<dyn ghrepo::Client + 'a>,
+    gist_client: Rc<dyn ghgist::Client + 'a>,
     job: &'a EvaluationJob,
     nix: Nix,
     tag_paths: &'a HashMap<String, Vec<String>>,
@@ -44,8 +45,8 @@ pub struct NixpkgsStrategy<'a> {
 impl<'a> NixpkgsStrategy<'a> {
     #[allow(clippy::too_many_arguments)]
     pub fn new(
-        repo_client: &'a ghrepo::Client<'a>,
-        gist_client: &'a ghgist::Client<'a>,
+        repo_client: Rc<dyn ghrepo::Client + 'a>,
+        gist_client: Rc<dyn ghgist::Client + 'a>,
         job: &'a EvaluationJob,
         nix: Nix,
         tag_paths: &'a HashMap<String, Vec<String>>,
@@ -75,7 +76,7 @@ impl<'a> NixpkgsStrategy<'a> {
 
         if darwin {
             update_labels(
-                &self.repo_client,
+                self.repo_client.as_ref(),
                 &self.job.pr,
                 &[String::from("6.topic: darwin")],
                 &[],
@@ -92,7 +93,7 @@ impl<'a> NixpkgsStrategy<'a> {
             }
 
             update_labels(
-                &self.repo_client,
+                self.repo_client.as_ref(),
                 &self.job.pr,
                 &tagger.tags_to_add(),
                 &tagger.tags_to_remove(),
@@ -119,7 +120,7 @@ impl<'a> NixpkgsStrategy<'a> {
                 stdenvtagger.changed(stdenvs.changed());
             }
             update_labels(
-                &self.repo_client,
+                self.repo_client.as_ref(),
                 &self.job.pr,
                 &stdenvtagger.tags_to_add(),
                 &stdenvtagger.tags_to_remove(),
@@ -199,7 +200,7 @@ impl<'a> NixpkgsStrategy<'a> {
                 let mut addremovetagger = PkgsAddedRemovedTagger::new();
                 addremovetagger.changed(&removed, &added);
                 update_labels(
-                    &self.repo_client,
+                    self.repo_client.as_ref(),
                     &self.job.pr,
                     &addremovetagger.tags_to_add(),
                     &addremovetagger.tags_to_remove(),
@@ -226,7 +227,7 @@ impl<'a> NixpkgsStrategy<'a> {
             }
 
             update_labels(
-                &self.repo_client,
+                self.repo_client.as_ref(),
                 &self.job.pr,
                 &rebuild_tags.tags_to_add(),
                 &rebuild_tags.tags_to_remove(),
@@ -237,7 +238,7 @@ impl<'a> NixpkgsStrategy<'a> {
 
     fn gist_changed_paths(&self, attrs: &[PackageArch]) -> Option<String> {
         make_gist(
-            self.gist_client,
+            self.gist_client.as_ref(),
             "Changed Paths",
             Some("".to_owned()),
             attrs
@@ -263,7 +264,7 @@ impl<'a> NixpkgsStrategy<'a> {
             );
 
             let gist_url = make_gist(
-                self.gist_client,
+                self.gist_client.as_ref(),
                 "Potential Maintainers",
                 Some("".to_owned()),
                 match m {
@@ -296,15 +297,15 @@ impl<'a> NixpkgsStrategy<'a> {
             status.set(hubcaps::statuses::State::Success)?;
 
             if let Ok(ref maint) = m {
-                request_reviews(&self.repo_client, &self.job.pr, &maint);
+                request_reviews(self.repo_client.as_ref(), &self.job.pr, &maint);
                 let mut maint_tagger = MaintainerPRTagger::new();
                 let issue = self
                     .repo_client
                     .get_issue(self.job.pr.number)
                     .map_err(|_e| Error::Fail(String::from("Failed to retrieve issue")))?;
                 maint_tagger.record_maintainer(&issue.user.login, &maint.maintainers_by_package());
                 update_labels(
-                    &self.repo_client,
+                    self.repo_client.as_ref(),
                     &self.job.pr,
                     &maint_tagger.tags_to_add(),
                     &maint_tagger.tags_to_remove(),
@@ -360,7 +361,7 @@ impl<'a> NixpkgsStrategy<'a> {
                 }
                 Err(out) => {
                     status.set_url(make_gist(
-                        self.gist_client,
+                        self.gist_client.as_ref(),
                         "Meta Check",
                         None,
                         out.display(),
@@ -416,7 +417,7 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> {
 
     fn merge_conflict(&mut self) {
         update_labels(
-            &self.repo_client,
+            self.repo_client.as_ref(),
             &self.job.pr,
             &["2.status: merge conflict".to_owned()],
             &[],
@@ -425,7 +426,7 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> {
 
     fn after_merge(&mut self, status: &mut CommitStatus) -> StepResult<()> {
         update_labels(
-            &self.repo_client,
+            self.repo_client.as_ref(),
             &self.job.pr,
             &[],
             &["2.status: merge conflict".to_owned()],
@@ -593,7 +594,7 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> {
 }
 
 fn request_reviews(
-    repo_client: &ghrepo::Client<'_>,
+    repo_client: &dyn ghrepo::Client,
     pr: &Pr,
     impacted_maintainers: &maintainers::ImpactedMaintainers,
 ) {

ofborg/src/tasks/evaluate.rs

@@ -15,6 +15,7 @@ use crate::worker;
 
 use std::collections::HashMap;
 use std::path::Path;
+use std::rc::Rc;
 use std::sync::RwLock;
 use std::time::Instant;
 
@@ -92,7 +93,7 @@ impl<E: stats::SysEvents + 'static> worker::SimpleWorker for EvaluationWorker<E>
             .for_repo(&job.repo.owner, &job.repo.name)
             .expect("Failed to get a github client token");
 
-        OneEval::new(
+        let mut eval = OneEval::new(
             github_client,
             &self.github,
             &self.nix,
@@ -102,14 +103,14 @@ impl<E: stats::SysEvents + 'static> worker::SimpleWorker for EvaluationWorker<E>
             &self.tag_paths,
             &self.cloner,
             job,
-        )
-        .worker_actions()
+        );
+        eval.worker_actions()
     }
 }
 
 struct OneEval<'a, E> {
-    repo_client: ghrepo::Client<'a>,
-    gist_client: ghgist::Client<'a>,
+    repo_client: Rc<dyn ghrepo::Client + 'a>,
+    gist_client: Rc<dyn ghgist::Client + 'a>,
     nix: &'a nix::Nix,
     acl: &'a ACL,
     events: &'a mut E,
@@ -132,11 +133,11 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
         cloner: &'a checkout::CachedCloner,
         job: &'a evaluationjob::EvaluationJob,
     ) -> OneEval<'a, E> {
-        let gist_client = ghgist::Client::new(client_legacy);
-        let repo_client = ghrepo::Client::new(client_app, &job.repo);
+        let gist_client = ghgist::Hubcaps::new(client_legacy);
+        let repo_client = ghrepo::Hubcaps::new(client_app, &job.repo);
         OneEval {
-            repo_client,
-            gist_client,
+            repo_client: Rc::new(repo_client),
+            gist_client: Rc::new(gist_client),
             nix,
             acl,
             events,
@@ -196,7 +197,12 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
             EvalWorkerError::EvalError(eval::Error::FailWithGist(msg, filename, content)) => self
                 .update_status(
                     msg,
-                    make_gist(&self.gist_client, &filename, Some("".to_owned()), content),
+                    make_gist(
+                        self.gist_client.as_ref(),
+                        &filename,
+                        Some("".to_owned()),
+                        content,
+                    ),
                     hubcaps::statuses::State::Failure,
                 ),
             EvalWorkerError::EvalError(eval::Error::CommitStatusWrite(e)) => Err(e),
@@ -229,7 +235,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
                     cswerr
                 );
                 update_labels(
-                    &self.repo_client,
+                    self.repo_client.as_ref(),
                     &self.job.pr,
                     &[String::from("ofborg-internal-error")],
                     &[],
@@ -274,8 +280,8 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
 
         let mut evaluation_strategy: Box<dyn eval::EvaluationStrategy> = if job.is_nixpkgs() {
             Box::new(eval::NixpkgsStrategy::new(
-                &self.repo_client,
-                &self.gist_client,
+                self.repo_client.clone(),
+                self.gist_client.clone(),
                 &job,
                 self.nix.clone(),
                 &self.tag_paths,
@@ -388,7 +394,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
                     Err(mut out) => {
                         state = hubcaps::statuses::State::Failure;
                         gist_url = make_gist(
-                            &self.gist_client,
+                            self.gist_client.as_ref(),
                             &check.name(),
                             Some(format!("{:?}", state)),
                             file_to_str(&mut out),
@@ -474,7 +480,7 @@ fn schedule_builds(
 }
 
 pub fn make_gist(
-    gist_client: &ghgist::Client,
+    gist_client: &dyn ghgist::Client,
     name: &str,
     description: Option<String>,
     contents: String,
@@ -500,7 +506,7 @@ pub fn make_gist(
     )
 }
 
-pub fn update_labels(repo_client: &ghrepo::Client, pr: &Pr, add: &[String], remove: &[String]) {
+pub fn update_labels(repo_client: &dyn ghrepo::Client, pr: &Pr, add: &[String], remove: &[String]) {
     let issue = repo_client
         .get_issue(pr.number)
         .expect("Failed to get issue");