From 2c69938413805f49b7fb5e636e7b035f972416d5 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Tue, 3 Nov 2020 22:31:38 +0100 Subject: [PATCH 01/12] eval: partially split up github interactions --- ofborg/src/ghrepo.rs | 50 ++++++++++++++++++++++++++ ofborg/src/lib.rs | 1 + ofborg/src/tasks/evaluate.rs | 68 ++++++++++++++++-------------------- 3 files changed, 81 insertions(+), 38 deletions(-) create mode 100644 ofborg/src/ghrepo.rs diff --git a/ofborg/src/ghrepo.rs b/ofborg/src/ghrepo.rs new file mode 100644 index 00000000..d4700574 --- /dev/null +++ b/ofborg/src/ghrepo.rs @@ -0,0 +1,50 @@ +use crate::commitstatus; +use crate::message; + +use hubcaps::checks::{CheckRun, CheckRunOptions}; +use hubcaps::issues::IssueRef; +use hubcaps::repositories::Repository; +use hubcaps::statuses::{Status, StatusOptions}; +use hubcaps::Github; + +pub struct Client<'a> { + repo: Repository<'a>, +} + +impl<'a> Client<'a> { + pub fn new(github: &'a Github, repo: &message::Repo) -> Self { + let repo = github.repo(repo.owner.clone(), repo.name.clone()); + Client { repo } + } + + pub fn get_repo(&self) -> &Repository<'a> { + &self.repo + } + + pub fn get_issue_ref(&self, number: u64) -> IssueRef { + self.repo.issue(number) + } + + pub fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result { + self.repo.statuses().create(sha, status) + } + + pub fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result { + self.repo.checkruns().create(&check) + } + + pub fn create_commitstatus( + &self, + pr: &message::Pr, + context: String, + description: String, + ) -> commitstatus::CommitStatus { + commitstatus::CommitStatus::new( + self.repo.statuses(), + pr.head_sha.clone(), + context, + description, + None, + ) + } +} diff --git a/ofborg/src/lib.rs b/ofborg/src/lib.rs index 35928c3c..28c687d4 100644 --- a/ofborg/src/lib.rs +++ b/ofborg/src/lib.rs @@ -28,6 +28,7 @@ pub mod easylapin; pub mod evalchecker; pub mod files; pub mod ghevent; +pub mod ghrepo; pub mod locks; pub mod maintainers; pub mod message; diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 82c962c4..37cd42bb 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -1,9 +1,10 @@ /// This is what evaluates every pull-request use crate::acl::ACL; use crate::checkout; -use crate::commitstatus::{CommitStatus, CommitStatusError}; +use crate::commitstatus::CommitStatusError; use crate::config::GithubAppVendingMachine; use crate::files::file_to_str; +use crate::ghrepo; use crate::message::{buildjob, evaluationjob}; use crate::nix; use crate::stats::{self, Event}; @@ -108,8 +109,7 @@ impl worker::SimpleWorker for EvaluationWorker } struct OneEval<'a, E> { - client_app: &'a hubcaps::Github, - repo: hubcaps::repositories::Repository<'a>, + repo_client: ghrepo::Client<'a>, gists: Gists<'a>, nix: &'a nix::Nix, acl: &'a ACL, @@ -135,10 +135,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { ) -> OneEval<'a, E> { let gists = client_legacy.gists(); - let repo = client_app.repo(job.repo.owner.clone(), job.repo.name.clone()); + let repo_client = ghrepo::Client::new(client_app, &job.repo); OneEval { - client_app, - repo, + repo_client, gists, nix, acl, @@ -183,11 +182,10 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { &self.job.pr.number, &self.job.pr.head_sha, &description ); - self.repo - .statuses() - .create(&self.job.pr.head_sha, &builder.build()) - .map(|_| ()) - .map_err(|e| CommitStatusError::from(e)) + self.repo_client + .create_status(&self.job.pr.head_sha, &builder.build()) + .map_err(|e| CommitStatusError::from(e))?; + Ok(()) } fn make_gist( @@ -241,7 +239,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { "Internal error writing commit status: {:?}, marking internal error", cswerr ); - let issue_ref = self.repo.issue(self.job.pr.number); + let issue_ref = self.repo_client.get_issue_ref(self.job.pr.number); update_labels(&issue_ref, &[String::from("ofborg-internal-error")], &[]); self.actions().skip(&self.job) @@ -253,15 +251,10 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { #[allow(clippy::cognitive_complexity)] fn evaluate_job(&mut self) -> Result { let job = self.job; - let repo = self - .client_app - .repo(self.job.repo.owner.clone(), self.job.repo.name.clone()); - let pulls = repo.pulls(); - let pull = pulls.get(job.pr.number); - let issue_ref = repo.issue(job.pr.number); let issue: Issue; let auto_schedule_build_archs: Vec; + let issue_ref = self.repo_client.get_issue_ref(job.pr.number); match issue_ref.get() { Ok(iss) => { if iss.state == "closed" { @@ -290,6 +283,11 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { } }; + // TODO don't pass hubcaps types directly + let repo = self.repo_client.get_repo(); + let pulls = repo.pulls(); + let pull = pulls.get(job.pr.number); + let mut evaluation_strategy: Box = if job.is_nixpkgs() { Box::new(eval::NixpkgsStrategy::new( &job, @@ -305,12 +303,10 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { Box::new(eval::GenericStrategy::new()) }; - let mut overall_status = CommitStatus::new( - repo.statuses(), - job.pr.head_sha.clone(), - "grahamcofborg-eval".to_owned(), - "Starting".to_owned(), - None, + let mut overall_status = self.repo_client.create_commitstatus( + &job.pr, + "grahamcofborg-eval".to_string(), + "Starting".to_string(), ); overall_status.set_with_description("Starting", hubcaps::statuses::State::Pending)?; @@ -389,13 +385,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { .evaluation_checks() .into_iter() .map(|check| { - let mut status = CommitStatus::new( - repo.statuses(), - job.pr.head_sha.clone(), - check.name(), - check.cli_cmd(), - None, - ); + let mut status = + self.repo_client + .create_commitstatus(&job.pr, check.name(), check.cli_cmd()); status .set(hubcaps::statuses::State::Pending) @@ -438,7 +430,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { let complete = evaluation_strategy .all_evaluations_passed(&Path::new(&refpath), &mut overall_status)?; - send_check_statuses(complete.checks, &repo); + self.send_check_statuses(complete.checks); response.extend(schedule_builds(complete.builds, auto_schedule_build_archs)); overall_status.set_with_description("^.^!", hubcaps::statuses::State::Success)?; @@ -452,13 +444,13 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { info!("Evaluations done!"); Ok(self.actions().done(&job, response)) } -} -fn send_check_statuses(checks: Vec, repo: &hubcaps::repositories::Repository) { - for check in checks { - match repo.checkruns().create(&check) { - Ok(_) => debug!("Sent check update"), - Err(e) => warn!("Failed to send check update: {:?}", e), + fn send_check_statuses(&self, checks: Vec) { + for check in checks { + match self.repo_client.create_checkrun(&check) { + Ok(_) => debug!("Sent check update"), + Err(e) => warn!("Failed to send check update: {:?}", e), + } } } } From f9c6ecdb6fb3a7f28e6ee82fef8f33e13300eaee Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sat, 7 Nov 2020 11:44:06 +0100 Subject: [PATCH 02/12] nixpkgs: split up review requests --- ofborg/src/ghrepo.rs | 16 ++++++++++ ofborg/src/tasks/eval/nixpkgs.rs | 53 +++++++++++++++++--------------- ofborg/src/tasks/evaluate.rs | 9 +----- 3 files changed, 46 insertions(+), 32 deletions(-) diff --git a/ofborg/src/ghrepo.rs b/ofborg/src/ghrepo.rs index d4700574..80f4fc93 100644 --- a/ofborg/src/ghrepo.rs +++ b/ofborg/src/ghrepo.rs @@ -3,7 +3,9 @@ use crate::message; use hubcaps::checks::{CheckRun, CheckRunOptions}; use hubcaps::issues::IssueRef; +use hubcaps::pulls::Pull; use hubcaps::repositories::Repository; +use hubcaps::review_requests::ReviewRequestOptions; use hubcaps::statuses::{Status, StatusOptions}; use hubcaps::Github; @@ -25,6 +27,20 @@ impl<'a> Client<'a> { self.repo.issue(number) } + pub fn get_pull(&self, number: u64) -> hubcaps::Result { + let pulls = self.repo.pulls(); + pulls.get(number).get() + } + + pub fn create_review_request( + &self, + number: u64, + review_request: &ReviewRequestOptions, + ) -> hubcaps::Result { + let pulls = self.repo.pulls(); + pulls.get(number).review_requests().create(review_request) + } + pub fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result { self.repo.statuses().create(sha, status) } diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index 47998833..a8888cf3 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/ofborg/src/tasks/eval/nixpkgs.rs @@ -2,9 +2,11 @@ use crate::checkout::CachedProjectCo; use crate::commentparser::Subset; use crate::commitstatus::CommitStatus; use crate::evalchecker::EvalChecker; +use crate::ghrepo; use crate::maintainers::{self, ImpactedMaintainers}; use crate::message::buildjob::BuildJob; use crate::message::evaluationjob::EvaluationJob; +use crate::message::Pr; use crate::nix::{self, Nix}; use crate::nixenv::HydraNixEnv; use crate::outpathdiff::{OutPathDiff, PackageArch}; @@ -22,7 +24,7 @@ use std::path::Path; use chrono::Utc; use hubcaps::checks::{CheckRunOptions, CheckRunState, Conclusion, Output}; use hubcaps::gists::Gists; -use hubcaps::issues::{Issue, IssueRef}; +use hubcaps::issues::IssueRef; use hubcaps::repositories::Repository; use tracing::{info, warn}; use uuid::Uuid; @@ -30,9 +32,8 @@ use uuid::Uuid; static MAINTAINER_REVIEW_MAX_CHANGED_PATHS: usize = 64; pub struct NixpkgsStrategy<'a> { + repo_client: &'a ghrepo::Client<'a>, job: &'a EvaluationJob, - pull: &'a hubcaps::pulls::PullRequest<'a>, - issue: &'a Issue, issue_ref: &'a IssueRef<'a>, repo: &'a Repository<'a>, gists: &'a Gists<'a>, @@ -47,9 +48,8 @@ pub struct NixpkgsStrategy<'a> { impl<'a> NixpkgsStrategy<'a> { #[allow(clippy::too_many_arguments)] pub fn new( + repo_client: &'a ghrepo::Client<'a>, job: &'a EvaluationJob, - pull: &'a hubcaps::pulls::PullRequest, - issue: &'a Issue, issue_ref: &'a IssueRef, repo: &'a Repository, gists: &'a Gists, @@ -57,9 +57,8 @@ impl<'a> NixpkgsStrategy<'a> { tag_paths: &'a HashMap>, ) -> NixpkgsStrategy<'a> { Self { + repo_client, job, - pull, - issue, issue_ref, repo, gists, @@ -298,10 +297,14 @@ impl<'a> NixpkgsStrategy<'a> { status.set(hubcaps::statuses::State::Success)?; if let Ok(ref maint) = m { - request_reviews(&maint, &self.pull); + request_reviews(&self.repo_client, &self.job.pr, &maint); let mut maint_tagger = MaintainerPRTagger::new(); - maint_tagger - .record_maintainer(&self.issue.user.login, &maint.maintainers_by_package()); + let issue = self + .repo_client + .get_issue_ref(self.job.pr.number) + .get() + .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.issue_ref, &maint_tagger.tags_to_add(), @@ -584,26 +587,28 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { } } -fn request_reviews(maint: &maintainers::ImpactedMaintainers, pull: &hubcaps::pulls::PullRequest) { - let pull_meta = pull.get(); - - if maint.maintainers().len() < 10 { - for maintainer in maint.maintainers() { - if let Ok(meta) = &pull_meta { +fn request_reviews( + repo_client: &ghrepo::Client<'_>, + pr: &Pr, + impacted_maintainers: &maintainers::ImpactedMaintainers, +) { + if impacted_maintainers.maintainers().len() < 10 { + for maintainer in impacted_maintainers.maintainers() { + if let Ok(pull) = repo_client.get_pull(pr.number) { // GitHub doesn't let us request a review from the PR author, so // we silently skip them. - if meta.user.login.to_ascii_lowercase() == maintainer.to_ascii_lowercase() { + if pull.user.login.to_ascii_lowercase() == maintainer.to_ascii_lowercase() { continue; } } - if let Err(e) = - pull.review_requests() - .create(&hubcaps::review_requests::ReviewRequestOptions { - reviewers: vec![maintainer.clone()], - team_reviewers: vec![], - }) - { + if let Err(e) = repo_client.create_review_request( + pr.number, + &hubcaps::review_requests::ReviewRequestOptions { + reviewers: vec![maintainer.clone()], + team_reviewers: vec![], + }, + ) { warn!("Failure requesting a review from {}: {:?}", maintainer, e,); } } diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 37cd42bb..83023393 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -19,7 +19,6 @@ use std::time::Instant; use hubcaps::checks::CheckRunOptions; use hubcaps::gists::Gists; -use hubcaps::issues::Issue; use tracing::{debug, debug_span, error, info, warn}; pub struct EvaluationWorker { @@ -251,7 +250,6 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { #[allow(clippy::cognitive_complexity)] fn evaluate_job(&mut self) -> Result { let job = self.job; - let issue: Issue; let auto_schedule_build_archs: Vec; let issue_ref = self.repo_client.get_issue_ref(job.pr.number); @@ -271,8 +269,6 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { &job.repo.full_name, ); } - - issue = iss; } Err(e) => { @@ -285,14 +281,11 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { // TODO don't pass hubcaps types directly let repo = self.repo_client.get_repo(); - let pulls = repo.pulls(); - let pull = pulls.get(job.pr.number); let mut evaluation_strategy: Box = if job.is_nixpkgs() { Box::new(eval::NixpkgsStrategy::new( + &self.repo_client, &job, - &pull, - &issue, &issue_ref, &repo, &self.gists, From 5fc83865c592e63ac32ce8a2c352ea62f44a79e2 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sat, 7 Nov 2020 11:53:22 +0100 Subject: [PATCH 03/12] nixpkgs: use create_commitstatus --- ofborg/src/ghrepo.rs | 7 ++----- ofborg/src/tasks/eval/nixpkgs.rs | 19 ++++++------------- ofborg/src/tasks/evaluate.rs | 14 +++++++------- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/ofborg/src/ghrepo.rs b/ofborg/src/ghrepo.rs index 80f4fc93..01e6ad9a 100644 --- a/ofborg/src/ghrepo.rs +++ b/ofborg/src/ghrepo.rs @@ -19,10 +19,6 @@ impl<'a> Client<'a> { Client { repo } } - pub fn get_repo(&self) -> &Repository<'a> { - &self.repo - } - pub fn get_issue_ref(&self, number: u64) -> IssueRef { self.repo.issue(number) } @@ -54,13 +50,14 @@ impl<'a> Client<'a> { pr: &message::Pr, context: String, description: String, + gist_url: Option, ) -> commitstatus::CommitStatus { commitstatus::CommitStatus::new( self.repo.statuses(), pr.head_sha.clone(), context, description, - None, + gist_url, ) } } diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index a8888cf3..b1328876 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/ofborg/src/tasks/eval/nixpkgs.rs @@ -25,7 +25,6 @@ use chrono::Utc; use hubcaps::checks::{CheckRunOptions, CheckRunState, Conclusion, Output}; use hubcaps::gists::Gists; use hubcaps::issues::IssueRef; -use hubcaps::repositories::Repository; use tracing::{info, warn}; use uuid::Uuid; @@ -35,7 +34,6 @@ pub struct NixpkgsStrategy<'a> { repo_client: &'a ghrepo::Client<'a>, job: &'a EvaluationJob, issue_ref: &'a IssueRef<'a>, - repo: &'a Repository<'a>, gists: &'a Gists<'a>, nix: Nix, tag_paths: &'a HashMap>, @@ -51,7 +49,6 @@ impl<'a> NixpkgsStrategy<'a> { repo_client: &'a ghrepo::Client<'a>, job: &'a EvaluationJob, issue_ref: &'a IssueRef, - repo: &'a Repository, gists: &'a Gists, nix: Nix, tag_paths: &'a HashMap>, @@ -60,7 +57,6 @@ impl<'a> NixpkgsStrategy<'a> { repo_client, job, issue_ref, - repo, gists, nix, tag_paths, @@ -276,9 +272,8 @@ impl<'a> NixpkgsStrategy<'a> { "pull request has {} changed paths, skipping review requests", changed_paths.len() ); - let status = CommitStatus::new( - self.repo.statuses(), - self.job.pr.head_sha.clone(), + let status = self.repo_client.create_commitstatus( + &self.job.pr, String::from("grahamcofborg-eval-check-maintainers"), String::from("large change, skipping automatic review requests"), gist_url, @@ -287,9 +282,8 @@ impl<'a> NixpkgsStrategy<'a> { return Ok(()); } - let status = CommitStatus::new( - self.repo.statuses(), - self.job.pr.head_sha.clone(), + let status = self.repo_client.create_commitstatus( + &self.job.pr, String::from("grahamcofborg-eval-check-maintainers"), String::from("matching changed paths to changed attrs..."), gist_url, @@ -318,9 +312,8 @@ impl<'a> NixpkgsStrategy<'a> { fn check_meta_queue_builds(&self, dir: &Path) -> StepResult> { if let Some(ref possibly_touched_packages) = self.touched_packages { - let mut status = CommitStatus::new( - self.repo.statuses(), - self.job.pr.head_sha.clone(), + let mut status = self.repo_client.create_commitstatus( + &self.job.pr, String::from("grahamcofborg-eval-check-meta"), String::from("config.nix: checkMeta = true"), None, diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 83023393..d000981c 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -279,15 +279,11 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { } }; - // TODO don't pass hubcaps types directly - let repo = self.repo_client.get_repo(); - let mut evaluation_strategy: Box = if job.is_nixpkgs() { Box::new(eval::NixpkgsStrategy::new( &self.repo_client, &job, &issue_ref, - &repo, &self.gists, self.nix.clone(), &self.tag_paths, @@ -300,6 +296,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { &job.pr, "grahamcofborg-eval".to_string(), "Starting".to_string(), + None, ); overall_status.set_with_description("Starting", hubcaps::statuses::State::Pending)?; @@ -378,9 +375,12 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { .evaluation_checks() .into_iter() .map(|check| { - let mut status = - self.repo_client - .create_commitstatus(&job.pr, check.name(), check.cli_cmd()); + let mut status = self.repo_client.create_commitstatus( + &job.pr, + check.name(), + check.cli_cmd(), + None, + ); status .set(hubcaps::statuses::State::Pending) From 365eaf01f2cf4b59a6f67081bbb6c402e64ae7cc Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sat, 7 Nov 2020 12:24:14 +0100 Subject: [PATCH 04/12] nixpkgs: split up issue labeling --- ofborg/src/ghrepo.rs | 15 +++++++-- ofborg/src/tasks/eval/nixpkgs.rs | 45 +++++++++++++++----------- ofborg/src/tasks/evaluate.rs | 55 ++++++++++++++++++-------------- 3 files changed, 69 insertions(+), 46 deletions(-) diff --git a/ofborg/src/ghrepo.rs b/ofborg/src/ghrepo.rs index 01e6ad9a..a5d292ba 100644 --- a/ofborg/src/ghrepo.rs +++ b/ofborg/src/ghrepo.rs @@ -2,7 +2,8 @@ use crate::commitstatus; use crate::message; use hubcaps::checks::{CheckRun, CheckRunOptions}; -use hubcaps::issues::IssueRef; +use hubcaps::issues::Issue; +use hubcaps::labels::Label; use hubcaps::pulls::Pull; use hubcaps::repositories::Repository; use hubcaps::review_requests::ReviewRequestOptions; @@ -19,8 +20,8 @@ impl<'a> Client<'a> { Client { repo } } - pub fn get_issue_ref(&self, number: u64) -> IssueRef { - self.repo.issue(number) + pub fn get_issue(&self, number: u64) -> hubcaps::Result { + self.repo.issue(number).get() } pub fn get_pull(&self, number: u64) -> hubcaps::Result { @@ -28,6 +29,14 @@ impl<'a> Client<'a> { pulls.get(number).get() } + pub fn add_labels(&self, number: u64, labels: Vec<&str>) -> hubcaps::Result> { + self.repo.issue(number).labels().add(labels) + } + + pub fn remove_label(&self, number: u64, label: &str) -> hubcaps::Result<()> { + self.repo.issue(number).labels().remove(label) + } + pub fn create_review_request( &self, number: u64, diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index b1328876..7ba285bf 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/ofborg/src/tasks/eval/nixpkgs.rs @@ -24,7 +24,6 @@ use std::path::Path; use chrono::Utc; use hubcaps::checks::{CheckRunOptions, CheckRunState, Conclusion, Output}; use hubcaps::gists::Gists; -use hubcaps::issues::IssueRef; use tracing::{info, warn}; use uuid::Uuid; @@ -33,7 +32,6 @@ static MAINTAINER_REVIEW_MAX_CHANGED_PATHS: usize = 64; pub struct NixpkgsStrategy<'a> { repo_client: &'a ghrepo::Client<'a>, job: &'a EvaluationJob, - issue_ref: &'a IssueRef<'a>, gists: &'a Gists<'a>, nix: Nix, tag_paths: &'a HashMap>, @@ -48,7 +46,6 @@ impl<'a> NixpkgsStrategy<'a> { pub fn new( repo_client: &'a ghrepo::Client<'a>, job: &'a EvaluationJob, - issue_ref: &'a IssueRef, gists: &'a Gists, nix: Nix, tag_paths: &'a HashMap>, @@ -56,7 +53,6 @@ impl<'a> NixpkgsStrategy<'a> { Self { repo_client, job, - issue_ref, gists, nix, tag_paths, @@ -69,16 +65,21 @@ impl<'a> NixpkgsStrategy<'a> { fn tag_from_title(&self) { let darwin = self - .issue_ref - .get() - .map(|iss| { - iss.title.to_lowercase().contains("darwin") - || iss.title.to_lowercase().contains("macos") + .repo_client + .get_issue(self.job.pr.number) + .map(|issue| { + issue.title.to_lowercase().contains("darwin") + || issue.title.to_lowercase().contains("macos") }) .unwrap_or(false); if darwin { - update_labels(&self.issue_ref, &[String::from("6.topic: darwin")], &[]); + update_labels( + &self.repo_client, + &self.job.pr, + &[String::from("6.topic: darwin")], + &[], + ); } } @@ -91,7 +92,8 @@ impl<'a> NixpkgsStrategy<'a> { } update_labels( - &self.issue_ref, + &self.repo_client, + &self.job.pr, &tagger.tags_to_add(), &tagger.tags_to_remove(), ); @@ -117,7 +119,8 @@ impl<'a> NixpkgsStrategy<'a> { stdenvtagger.changed(stdenvs.changed()); } update_labels( - &self.issue_ref, + &self.repo_client, + &self.job.pr, &stdenvtagger.tags_to_add(), &stdenvtagger.tags_to_remove(), ); @@ -196,7 +199,8 @@ impl<'a> NixpkgsStrategy<'a> { let mut addremovetagger = PkgsAddedRemovedTagger::new(); addremovetagger.changed(&removed, &added); update_labels( - &self.issue_ref, + &self.repo_client, + &self.job.pr, &addremovetagger.tags_to_add(), &addremovetagger.tags_to_remove(), ); @@ -222,7 +226,8 @@ impl<'a> NixpkgsStrategy<'a> { } update_labels( - &self.issue_ref, + &self.repo_client, + &self.job.pr, &rebuild_tags.tags_to_add(), &rebuild_tags.tags_to_remove(), ); @@ -295,12 +300,12 @@ impl<'a> NixpkgsStrategy<'a> { let mut maint_tagger = MaintainerPRTagger::new(); let issue = self .repo_client - .get_issue_ref(self.job.pr.number) - .get() + .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.issue_ref, + &self.repo_client, + &self.job.pr, &maint_tagger.tags_to_add(), &maint_tagger.tags_to_remove(), ); @@ -406,7 +411,8 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { fn merge_conflict(&mut self) { update_labels( - &self.issue_ref, + &self.repo_client, + &self.job.pr, &["2.status: merge conflict".to_owned()], &[], ); @@ -414,7 +420,8 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { fn after_merge(&mut self, status: &mut CommitStatus) -> StepResult<()> { update_labels( - &self.issue_ref, + &self.repo_client, + &self.job.pr, &[], &["2.status: merge conflict".to_owned()], ); diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index d000981c..48d8af54 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -5,7 +5,7 @@ use crate::commitstatus::CommitStatusError; use crate::config::GithubAppVendingMachine; use crate::files::file_to_str; use crate::ghrepo; -use crate::message::{buildjob, evaluationjob}; +use crate::message::{buildjob, evaluationjob, Pr}; use crate::nix; use crate::stats::{self, Event}; use crate::systems; @@ -238,8 +238,12 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { "Internal error writing commit status: {:?}, marking internal error", cswerr ); - let issue_ref = self.repo_client.get_issue_ref(self.job.pr.number); - update_labels(&issue_ref, &[String::from("ofborg-internal-error")], &[]); + update_labels( + &self.repo_client, + &self.job.pr, + &[String::from("ofborg-internal-error")], + &[], + ); self.actions().skip(&self.job) } @@ -252,20 +256,19 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { let job = self.job; let auto_schedule_build_archs: Vec; - let issue_ref = self.repo_client.get_issue_ref(job.pr.number); - match issue_ref.get() { - Ok(iss) => { - if iss.state == "closed" { + match self.repo_client.get_issue(job.pr.number) { + Ok(issue) => { + if issue.state == "closed" { self.events.notify(Event::IssueAlreadyClosed); info!("Skipping {} because it is closed", job.pr.number); return Ok(self.actions().skip(&job)); } - if issue_is_wip(&iss) { + if issue_is_wip(&issue) { auto_schedule_build_archs = vec![]; } else { auto_schedule_build_archs = self.acl.build_job_architectures_for_user_repo( - &iss.user.login, + &issue.user.login, &job.repo.full_name, ); } @@ -283,7 +286,6 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { Box::new(eval::NixpkgsStrategy::new( &self.repo_client, &job, - &issue_ref, &self.gists, self.nix.clone(), &self.tag_paths, @@ -507,9 +509,10 @@ pub fn make_gist<'a>( ) } -pub fn update_labels(issueref: &hubcaps::issues::IssueRef, add: &[String], remove: &[String]) { - let l = issueref.labels(); - let issue = issueref.get().expect("Failed to get issue"); +pub fn update_labels(repo_client: &ghrepo::Client, pr: &Pr, add: &[String], remove: &[String]) { + let issue = repo_client + .get_issue(pr.number) + .expect("Failed to get issue"); let existing: Vec = issue.labels.iter().map(|l| l.name.clone()).collect(); @@ -530,20 +533,24 @@ pub fn update_labels(issueref: &hubcaps::issues::IssueRef, add: &[String], remov issue.number, to_add, to_remove, existing ); - l.add(to_add.clone()).unwrap_or_else(|e| { - panic!( - "Failed to add labels {:?} to issue #{}: {:?}", - to_add, issue.number, e - ) - }); - - for label in to_remove { - l.remove(&label).unwrap_or_else(|e| { + repo_client + .add_labels(pr.number, to_add.clone()) + .unwrap_or_else(|e| { panic!( - "Failed to remove label {:?} from issue #{}: {:?}", - label, issue.number, e + "Failed to add labels {:?} to issue #{}: {:?}", + to_add, issue.number, e ) }); + + for label in to_remove { + repo_client + .remove_label(pr.number, &label) + .unwrap_or_else(|e| { + panic!( + "Failed to remove label {:?} from issue #{}: {:?}", + label, issue.number, e + ) + }); } } From 9756e1544beade00bdee045fe166a8f266691da8 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sat, 7 Nov 2020 21:13:34 +0100 Subject: [PATCH 05/12] eval: split up gist creation --- ofborg/src/ghgist.rs | 15 +++++++++++++++ ofborg/src/lib.rs | 1 + ofborg/src/tasks/eval/nixpkgs.rs | 19 +++++++++++------- ofborg/src/tasks/evaluate.rs | 33 ++++++++++++-------------------- 4 files changed, 40 insertions(+), 28 deletions(-) create mode 100644 ofborg/src/ghgist.rs diff --git a/ofborg/src/ghgist.rs b/ofborg/src/ghgist.rs new file mode 100644 index 00000000..2d87f311 --- /dev/null +++ b/ofborg/src/ghgist.rs @@ -0,0 +1,15 @@ +use hubcaps::gists::{Gist, GistOptions}; + +pub struct Client<'a> { + github: &'a hubcaps::Github, +} + +impl<'a> Client<'a> { + pub fn new(github: &'a hubcaps::Github) -> Self { + Client { github } + } + + pub fn create_gist(&self, gist: &GistOptions) -> hubcaps::Result { + self.github.gists().create(gist) + } +} diff --git a/ofborg/src/lib.rs b/ofborg/src/lib.rs index 28c687d4..0272204c 100644 --- a/ofborg/src/lib.rs +++ b/ofborg/src/lib.rs @@ -28,6 +28,7 @@ pub mod easylapin; pub mod evalchecker; pub mod files; pub mod ghevent; +pub mod ghgist; pub mod ghrepo; pub mod locks; pub mod maintainers; diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index 7ba285bf..cf6bcb8b 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/ofborg/src/tasks/eval/nixpkgs.rs @@ -2,6 +2,7 @@ use crate::checkout::CachedProjectCo; use crate::commentparser::Subset; use crate::commitstatus::CommitStatus; use crate::evalchecker::EvalChecker; +use crate::ghgist; use crate::ghrepo; use crate::maintainers::{self, ImpactedMaintainers}; use crate::message::buildjob::BuildJob; @@ -23,7 +24,6 @@ use std::path::Path; use chrono::Utc; use hubcaps::checks::{CheckRunOptions, CheckRunState, Conclusion, Output}; -use hubcaps::gists::Gists; use tracing::{info, warn}; use uuid::Uuid; @@ -31,8 +31,8 @@ static MAINTAINER_REVIEW_MAX_CHANGED_PATHS: usize = 64; pub struct NixpkgsStrategy<'a> { repo_client: &'a ghrepo::Client<'a>, + gist_client: &'a ghgist::Client<'a>, job: &'a EvaluationJob, - gists: &'a Gists<'a>, nix: Nix, tag_paths: &'a HashMap>, stdenv_diff: Option, @@ -45,15 +45,15 @@ impl<'a> NixpkgsStrategy<'a> { #[allow(clippy::too_many_arguments)] pub fn new( repo_client: &'a ghrepo::Client<'a>, + gist_client: &'a ghgist::Client<'a>, job: &'a EvaluationJob, - gists: &'a Gists, nix: Nix, tag_paths: &'a HashMap>, ) -> NixpkgsStrategy<'a> { Self { repo_client, + gist_client, job, - gists, nix, tag_paths, stdenv_diff: None, @@ -237,7 +237,7 @@ impl<'a> NixpkgsStrategy<'a> { fn gist_changed_paths(&self, attrs: &[PackageArch]) -> Option { make_gist( - &self.gists, + self.gist_client, "Changed Paths", Some("".to_owned()), attrs @@ -263,7 +263,7 @@ impl<'a> NixpkgsStrategy<'a> { ); let gist_url = make_gist( - &self.gists, + self.gist_client, "Potential Maintainers", Some("".to_owned()), match m { @@ -359,7 +359,12 @@ impl<'a> NixpkgsStrategy<'a> { } } Err(out) => { - status.set_url(make_gist(&self.gists, "Meta Check", None, out.display())); + status.set_url(make_gist( + self.gist_client, + "Meta Check", + None, + out.display(), + )); status.set(hubcaps::statuses::State::Failure)?; Err(Error::Fail(String::from( "Failed to validate package metadata.", diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 48d8af54..105c7549 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -4,6 +4,7 @@ use crate::checkout; use crate::commitstatus::CommitStatusError; use crate::config::GithubAppVendingMachine; use crate::files::file_to_str; +use crate::ghgist; use crate::ghrepo; use crate::message::{buildjob, evaluationjob, Pr}; use crate::nix; @@ -18,7 +19,6 @@ use std::sync::RwLock; use std::time::Instant; use hubcaps::checks::CheckRunOptions; -use hubcaps::gists::Gists; use tracing::{debug, debug_span, error, info, warn}; pub struct EvaluationWorker { @@ -109,7 +109,7 @@ impl worker::SimpleWorker for EvaluationWorker struct OneEval<'a, E> { repo_client: ghrepo::Client<'a>, - gists: Gists<'a>, + gist_client: ghgist::Client<'a>, nix: &'a nix::Nix, acl: &'a ACL, events: &'a mut E, @@ -132,12 +132,11 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { cloner: &'a checkout::CachedCloner, job: &'a evaluationjob::EvaluationJob, ) -> OneEval<'a, E> { - let gists = client_legacy.gists(); - + let gist_client = ghgist::Client::new(client_legacy); let repo_client = ghrepo::Client::new(client_app, &job.repo); OneEval { repo_client, - gists, + gist_client, nix, acl, events, @@ -187,15 +186,6 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { Ok(()) } - fn make_gist( - &self, - filename: &str, - description: Option, - content: String, - ) -> Option { - make_gist(&self.gists, filename, description, content) - } - fn worker_actions(&mut self) -> worker::Actions { let eval_result = self.evaluate_job().map_err(|eval_error| match eval_error { // Handle error cases which expect us to post statuses @@ -206,7 +196,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { EvalWorkerError::EvalError(eval::Error::FailWithGist(msg, filename, content)) => self .update_status( msg, - self.make_gist(&filename, Some("".to_owned()), content), + make_gist(&self.gist_client, &filename, Some("".to_owned()), content), hubcaps::statuses::State::Failure, ), EvalWorkerError::EvalError(eval::Error::CommitStatusWrite(e)) => Err(e), @@ -285,8 +275,8 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { let mut evaluation_strategy: Box = if job.is_nixpkgs() { Box::new(eval::NixpkgsStrategy::new( &self.repo_client, + &self.gist_client, &job, - &self.gists, self.nix.clone(), &self.tag_paths, )) @@ -397,7 +387,8 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { } Err(mut out) => { state = hubcaps::statuses::State::Failure; - gist_url = self.make_gist( + gist_url = make_gist( + &self.gist_client, &check.name(), Some(format!("{:?}", state)), file_to_str(&mut out), @@ -482,8 +473,8 @@ fn schedule_builds( response } -pub fn make_gist<'a>( - gists: &hubcaps::gists::Gists<'a>, +pub fn make_gist( + gist_client: &ghgist::Client, name: &str, description: Option, contents: String, @@ -498,8 +489,8 @@ pub fn make_gist<'a>( ); Some( - gists - .create(&hubcaps::gists::GistOptions { + gist_client + .create_gist(&hubcaps::gists::GistOptions { description, public: Some(true), files, From 7589064d492dc308dba10dc31b4e3dcd5a2d22b9 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sun, 8 Nov 2020 14:12:29 +0100 Subject: [PATCH 06/12] eval: use trait for github interactions --- ofborg/src/ghgist.rs | 14 +++++++--- ofborg/src/ghrepo.rs | 45 ++++++++++++++++++++++++-------- ofborg/src/tasks/eval/nixpkgs.rs | 36 ++++++++++++------------- ofborg/src/tasks/evaluate.rs | 38 +++++++++++++++------------ 4 files changed, 84 insertions(+), 49 deletions(-) diff --git a/ofborg/src/ghgist.rs b/ofborg/src/ghgist.rs index 2d87f311..6175ac3f 100644 --- a/ofborg/src/ghgist.rs +++ b/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; +} + +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 { +impl Client for Hubcaps<'_> { + fn create_gist(&self, gist: &GistOptions) -> hubcaps::Result { self.github.gists().create(gist) } } diff --git a/ofborg/src/ghrepo.rs b/ofborg/src/ghrepo.rs index a5d292ba..ab40f43a 100644 --- a/ofborg/src/ghrepo.rs +++ b/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; + fn get_pull(&self, number: u64) -> hubcaps::Result; + fn add_labels(&self, number: u64, labels: Vec<&str>) -> hubcaps::Result>; + fn remove_label(&self, number: u64, label: &str) -> hubcaps::Result<()>; + fn create_review_request( + &self, + number: u64, + review_request: &ReviewRequestOptions, + ) -> hubcaps::Result; + fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result; + fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result; + fn create_commitstatus( + &self, + pr: &message::Pr, + context: String, + description: String, + gist_url: Option, + ) -> 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 { +impl Client for Hubcaps<'_> { + fn get_issue(&self, number: u64) -> hubcaps::Result { self.repo.issue(number).get() } - pub fn get_pull(&self, number: u64) -> hubcaps::Result { + fn get_pull(&self, number: u64) -> hubcaps::Result { let pulls = self.repo.pulls(); pulls.get(number).get() } - pub fn add_labels(&self, number: u64, labels: Vec<&str>) -> hubcaps::Result> { + fn add_labels(&self, number: u64, labels: Vec<&str>) -> hubcaps::Result> { 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 { + fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result { self.repo.statuses().create(sha, status) } - pub fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result { + fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result { self.repo.checkruns().create(&check) } - pub fn create_commitstatus( + fn create_commitstatus( &self, pr: &message::Pr, context: String, diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index cf6bcb8b..1987f4fa 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/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, + gist_client: Rc, job: &'a EvaluationJob, nix: Nix, tag_paths: &'a HashMap>, @@ -42,10 +43,9 @@ 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, + gist_client: Rc, job: &'a EvaluationJob, nix: Nix, tag_paths: &'a HashMap>, @@ -75,7 +75,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 +92,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 +119,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 +199,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 +226,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 +237,7 @@ impl<'a> NixpkgsStrategy<'a> { fn gist_changed_paths(&self, attrs: &[PackageArch]) -> Option { make_gist( - self.gist_client, + self.gist_client.as_ref(), "Changed Paths", Some("".to_owned()), attrs @@ -263,7 +263,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,7 +296,7 @@ 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 @@ -304,7 +304,7 @@ impl<'a> NixpkgsStrategy<'a> { .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 +360,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 +416,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 +425,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 +593,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, ) { diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 105c7549..0e982bf0 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/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 worker::SimpleWorker for EvaluationWorker .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 worker::SimpleWorker for EvaluationWorker &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, + gist_client: Rc, 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 = 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, 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"); From 3382a9c0414f8a7eed74fc4f74d821d0fdf50942 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Sun, 8 Nov 2020 16:58:15 +0100 Subject: [PATCH 07/12] commitstatus: use github trait --- ofborg/src/commitstatus.rs | 13 ++++++++----- ofborg/src/ghrepo.rs | 24 ------------------------ ofborg/src/tasks/eval/nixpkgs.rs | 15 +++++++++------ ofborg/src/tasks/evaluate.rs | 12 +++++++----- 4 files changed, 24 insertions(+), 40 deletions(-) diff --git a/ofborg/src/commitstatus.rs b/ofborg/src/commitstatus.rs index 8ee6dd64..f4fb3fab 100644 --- a/ofborg/src/commitstatus.rs +++ b/ofborg/src/commitstatus.rs @@ -1,7 +1,10 @@ +use crate::ghrepo; + +use std::rc::Rc; use tracing::warn; pub struct CommitStatus<'a> { - api: hubcaps::statuses::Statuses<'a>, + repo_client: Rc, sha: String, context: String, description: String, @@ -10,14 +13,14 @@ pub struct CommitStatus<'a> { impl<'a> CommitStatus<'a> { pub fn new( - api: hubcaps::statuses::Statuses<'a>, + repo_client: Rc, sha: String, context: String, description: String, url: Option, ) -> CommitStatus<'a> { let mut stat = CommitStatus { - api, + repo_client, sha, context, description, @@ -57,8 +60,8 @@ impl<'a> CommitStatus<'a> { self.description.clone() }; - self.api - .create( + self.repo_client + .create_status( self.sha.as_ref(), &hubcaps::statuses::StatusOptions::builder(state) .context(self.context.clone()) diff --git a/ofborg/src/ghrepo.rs b/ofborg/src/ghrepo.rs index ab40f43a..03df1bc8 100644 --- a/ofborg/src/ghrepo.rs +++ b/ofborg/src/ghrepo.rs @@ -1,4 +1,3 @@ -use crate::commitstatus; use crate::message; use hubcaps::checks::{CheckRun, CheckRunOptions}; @@ -22,13 +21,6 @@ pub trait Client { ) -> hubcaps::Result; fn create_status(&self, sha: &str, status: &StatusOptions) -> hubcaps::Result; fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result; - fn create_commitstatus( - &self, - pr: &message::Pr, - context: String, - description: String, - gist_url: Option, - ) -> commitstatus::CommitStatus; } pub struct Hubcaps<'a> { @@ -76,20 +68,4 @@ impl Client for Hubcaps<'_> { fn create_checkrun(&self, check: &CheckRunOptions) -> hubcaps::Result { self.repo.checkruns().create(&check) } - - fn create_commitstatus( - &self, - pr: &message::Pr, - context: String, - description: String, - gist_url: Option, - ) -> commitstatus::CommitStatus { - commitstatus::CommitStatus::new( - self.repo.statuses(), - pr.head_sha.clone(), - context, - description, - gist_url, - ) - } } diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index 1987f4fa..6fadd4e3 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/ofborg/src/tasks/eval/nixpkgs.rs @@ -277,8 +277,9 @@ impl<'a> NixpkgsStrategy<'a> { "pull request has {} changed paths, skipping review requests", changed_paths.len() ); - let status = self.repo_client.create_commitstatus( - &self.job.pr, + let status = CommitStatus::new( + self.repo_client.clone(), + self.job.pr.head_sha.clone(), String::from("grahamcofborg-eval-check-maintainers"), String::from("large change, skipping automatic review requests"), gist_url, @@ -287,8 +288,9 @@ impl<'a> NixpkgsStrategy<'a> { return Ok(()); } - let status = self.repo_client.create_commitstatus( - &self.job.pr, + let status = CommitStatus::new( + self.repo_client.clone(), + self.job.pr.head_sha.clone(), String::from("grahamcofborg-eval-check-maintainers"), String::from("matching changed paths to changed attrs..."), gist_url, @@ -317,8 +319,9 @@ impl<'a> NixpkgsStrategy<'a> { fn check_meta_queue_builds(&self, dir: &Path) -> StepResult> { if let Some(ref possibly_touched_packages) = self.touched_packages { - let mut status = self.repo_client.create_commitstatus( - &self.job.pr, + let mut status = CommitStatus::new( + self.repo_client.clone(), + self.job.pr.head_sha.clone(), String::from("grahamcofborg-eval-check-meta"), String::from("config.nix: checkMeta = true"), None, diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 0e982bf0..af86229b 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -1,7 +1,7 @@ /// This is what evaluates every pull-request use crate::acl::ACL; use crate::checkout; -use crate::commitstatus::CommitStatusError; +use crate::commitstatus::{CommitStatus, CommitStatusError}; use crate::config::GithubAppVendingMachine; use crate::files::file_to_str; use crate::ghgist; @@ -290,8 +290,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { Box::new(eval::GenericStrategy::new()) }; - let mut overall_status = self.repo_client.create_commitstatus( - &job.pr, + let mut overall_status = CommitStatus::new( + self.repo_client.clone(), + job.pr.head_sha.clone(), "grahamcofborg-eval".to_string(), "Starting".to_string(), None, @@ -373,8 +374,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { .evaluation_checks() .into_iter() .map(|check| { - let mut status = self.repo_client.create_commitstatus( - &job.pr, + let mut status = CommitStatus::new( + self.repo_client.clone(), + job.pr.head_sha.clone(), check.name(), check.cli_cmd(), None, From ce2f6be7f32e8682ef87e0375e0aa6aa70472c8f Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Mon, 9 Nov 2020 22:38:27 +0100 Subject: [PATCH 08/12] eval: move job to parameters --- ofborg/src/tasks/evaluate.rs | 73 +++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index af86229b..ae2db093 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -102,9 +102,9 @@ impl worker::SimpleWorker for EvaluationWorker &self.identity, &self.tag_paths, &self.cloner, - job, + &job, ); - eval.worker_actions() + eval.worker_actions(&job) } } @@ -117,7 +117,6 @@ struct OneEval<'a, E> { identity: &'a str, tag_paths: &'a HashMap>, cloner: &'a checkout::CachedCloner, - job: &'a evaluationjob::EvaluationJob, } impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { @@ -144,7 +143,6 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { identity, tag_paths, cloner, - job, } } @@ -154,6 +152,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { fn update_status( &self, + pr: &Pr, description: String, url: Option, state: hubcaps::statuses::State, @@ -178,36 +177,40 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { info!( "Updating status on {}:{} -> {}", - &self.job.pr.number, &self.job.pr.head_sha, &description + &pr.number, &pr.head_sha, &description ); self.repo_client - .create_status(&self.job.pr.head_sha, &builder.build()) + .create_status(&pr.head_sha, &builder.build()) .map_err(|e| CommitStatusError::from(e))?; Ok(()) } - fn worker_actions(&mut self) -> worker::Actions { - let eval_result = self.evaluate_job().map_err(|eval_error| match eval_error { - // Handle error cases which expect us to post statuses - // to github. Convert Eval Errors in to Result<_, CommitStatusWrite> - EvalWorkerError::EvalError(eval::Error::Fail(msg)) => { - self.update_status(msg, None, hubcaps::statuses::State::Failure) - } - EvalWorkerError::EvalError(eval::Error::FailWithGist(msg, filename, content)) => self - .update_status( - msg, - make_gist( - self.gist_client.as_ref(), - &filename, - Some("".to_owned()), - content, - ), - hubcaps::statuses::State::Failure, - ), - EvalWorkerError::EvalError(eval::Error::CommitStatusWrite(e)) => Err(e), - EvalWorkerError::CommitStatusWrite(e) => Err(e), - }); + fn worker_actions(&mut self, job: &evaluationjob::EvaluationJob) -> worker::Actions { + let eval_result = self + .evaluate_job(job) + .map_err(|eval_error| match eval_error { + // Handle error cases which expect us to post statuses + // to github. Convert Eval Errors in to Result<_, CommitStatusWrite> + EvalWorkerError::EvalError(eval::Error::Fail(msg)) => { + self.update_status(&job.pr, msg, None, hubcaps::statuses::State::Failure) + } + EvalWorkerError::EvalError(eval::Error::FailWithGist(msg, filename, content)) => { + self.update_status( + &job.pr, + msg, + make_gist( + self.gist_client.as_ref(), + &filename, + Some("".to_owned()), + content, + ), + hubcaps::statuses::State::Failure, + ) + } + EvalWorkerError::EvalError(eval::Error::CommitStatusWrite(e)) => Err(e), + EvalWorkerError::CommitStatusWrite(e) => Err(e), + }); match eval_result { Ok(eval_actions) => eval_actions, @@ -215,18 +218,18 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { // There was an error during eval, but we successfully // updated the PR. - self.actions().skip(&self.job) + self.actions().skip(&job) } Err(Err(CommitStatusError::ExpiredCreds(e))) => { error!("Failed writing commit status: creds expired: {:?}", e); - self.actions().retry_later(&self.job) + self.actions().retry_later(&job) } Err(Err(CommitStatusError::MissingSHA(e))) => { error!( "Failed writing commit status: commit sha was force-pushed away: {:?}", e ); - self.actions().skip(&self.job) + self.actions().skip(&job) } Err(Err(CommitStatusError::Error(cswerr))) => { @@ -236,20 +239,22 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { ); update_labels( self.repo_client.as_ref(), - &self.job.pr, + &job.pr, &[String::from("ofborg-internal-error")], &[], ); - self.actions().skip(&self.job) + self.actions().skip(&job) } } } // FIXME: remove with rust/cargo update #[allow(clippy::cognitive_complexity)] - fn evaluate_job(&mut self) -> Result { - let job = self.job; + fn evaluate_job( + &mut self, + job: &evaluationjob::EvaluationJob, + ) -> Result { let auto_schedule_build_archs: Vec; match self.repo_client.get_issue(job.pr.number) { From 30002f93722f65061c33c8e30832d105b41112c5 Mon Sep 17 00:00:00 2001 From: Daiderd Jordan Date: Tue, 24 Nov 2020 20:26:57 +0100 Subject: [PATCH 09/12] eval: add test for wip issues --- ofborg/src/tasks/evaluate.rs | 72 ++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index ae2db093..61526527 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -600,3 +600,75 @@ impl From for EvalWorkerError { EvalWorkerError::CommitStatusWrite(e) } } + +#[cfg(test)] +mod tests { + use super::*; + use hubcaps::issues::Issue; + use hubcaps::labels::Label; + use hubcaps::users::User; + + fn make_issue(number: u64, status: &str, title: &str, labels: Vec