From 90fda07cf259f282c0a43192e2c50dd627847b77 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Tue, 31 Mar 2020 19:05:18 -0400 Subject: [PATCH] commitstatus: handle errors intsead of expecting them (mostly) --- ofborg/src/tasks/eval/mod.rs | 9 +++++- ofborg/src/tasks/eval/nixpkgs.rs | 33 +++++++++++-------- ofborg/src/tasks/evaluate.rs | 55 ++++++++++++++++++++++++-------- 3 files changed, 69 insertions(+), 28 deletions(-) diff --git a/ofborg/src/tasks/eval/mod.rs b/ofborg/src/tasks/eval/mod.rs index f2e9006..3ed2376 100644 --- a/ofborg/src/tasks/eval/mod.rs +++ b/ofborg/src/tasks/eval/mod.rs @@ -6,7 +6,7 @@ mod generic; pub use self::generic::GenericStrategy; use hubcaps::checks::CheckRunOptions; use ofborg::checkout::CachedProjectCo; -use ofborg::commitstatus::CommitStatus; +use ofborg::commitstatus::{CommitStatus, CommitStatusError}; use ofborg::evalchecker::EvalChecker; use ofborg::message::buildjob::BuildJob; use std::path::Path; @@ -36,6 +36,13 @@ pub struct EvaluationComplete { #[derive(Debug)] pub enum Error { + CommitStatusWrite(CommitStatusError), Fail(String), FailWithGist(String, String, String), } + +impl From for Error { + fn from(e: CommitStatusError) -> Error { + Error::CommitStatusWrite(e) + } +} diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index 30fcc8b..baa3c80 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/ofborg/src/tasks/eval/nixpkgs.rs @@ -206,14 +206,18 @@ impl<'a> NixpkgsStrategy<'a> { } } - fn update_rebuild_labels(&self, dir: &Path, overall_status: &mut CommitStatus) { + fn update_rebuild_labels( + &self, + dir: &Path, + overall_status: &mut CommitStatus, + ) -> Result<(), Error> { if let Some(ref rebuildsniff) = self.outpath_diff { let mut rebuild_tags = RebuildTagger::new(); if let Some(attrs) = rebuildsniff.calculate_rebuild() { if !attrs.is_empty() { overall_status.set_url(self.gist_changed_paths(&attrs)); - self.record_impacted_maintainers(&dir, &attrs); + self.record_impacted_maintainers(&dir, &attrs)?; } rebuild_tags.parse_attrs(attrs); @@ -225,6 +229,7 @@ impl<'a> NixpkgsStrategy<'a> { &rebuild_tags.tags_to_remove(), ); } + Ok(()) } fn gist_changed_paths(&self, attrs: &[PackageArch]) -> Option { @@ -240,7 +245,7 @@ impl<'a> NixpkgsStrategy<'a> { ) } - fn record_impacted_maintainers(&self, dir: &Path, attrs: &[PackageArch]) { + fn record_impacted_maintainers(&self, dir: &Path, attrs: &[PackageArch]) -> Result<(), Error> { let changed_attributes = attrs .iter() .map(|attr| attr.package.split('.').collect::>()) @@ -271,7 +276,7 @@ impl<'a> NixpkgsStrategy<'a> { String::from("matching changed paths to changed attrs..."), gist_url, ); - status.set(hubcaps::statuses::State::Success); + status.set(hubcaps::statuses::State::Success)?; if let Ok(ref maint) = m { request_reviews(&maint, &self.pull); @@ -285,6 +290,8 @@ impl<'a> NixpkgsStrategy<'a> { ); } } + + Ok(()) } fn check_meta_queue_builds(&self, dir: &Path) -> StepResult> { @@ -296,7 +303,7 @@ impl<'a> NixpkgsStrategy<'a> { String::from("config.nix: checkMeta = true"), None, ); - status.set(hubcaps::statuses::State::Pending); + status.set(hubcaps::statuses::State::Pending)?; let nixenv = HydraNixEnv::new(self.nix.clone(), dir.to_path_buf(), true); match nixenv.execute_with_stats() { @@ -310,7 +317,7 @@ impl<'a> NixpkgsStrategy<'a> { try_build.dedup(); status.set_url(None); - status.set(hubcaps::statuses::State::Success); + status.set(hubcaps::statuses::State::Success)?; if !try_build.is_empty() && try_build.len() <= 10 { // In the case of trying to merge master in to @@ -332,7 +339,7 @@ impl<'a> NixpkgsStrategy<'a> { } Err(out) => { status.set_url(make_gist(&self.gists, "Meta Check", None, out.display())); - status.set(hubcaps::statuses::State::Failure); + status.set(hubcaps::statuses::State::Failure)?; Err(Error::Fail(String::from( "Failed to validate package metadata.", ))) @@ -354,13 +361,13 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { status.set_with_description( "Checking original stdenvs", hubcaps::statuses::State::Pending, - ); + )?; self.check_stdenvs_before(dir); status.set_with_description( "Checking original out paths", hubcaps::statuses::State::Pending, - ); + )?; self.check_outpaths_before(dir)?; Ok(()) @@ -396,10 +403,10 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { &["2.status: merge conflict".to_owned()], ); - status.set_with_description("Checking new stdenvs", hubcaps::statuses::State::Pending); + status.set_with_description("Checking new stdenvs", hubcaps::statuses::State::Pending)?; self.check_stdenvs_after(); - status.set_with_description("Checking new out paths", hubcaps::statuses::State::Pending); + status.set_with_description("Checking new out paths", hubcaps::statuses::State::Pending)?; self.check_outpaths_after()?; Ok(()) @@ -535,10 +542,10 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { status.set_with_description( "Calculating Changed Outputs", hubcaps::statuses::State::Pending, - ); + )?; self.update_new_package_labels(); - self.update_rebuild_labels(&dir, status); + self.update_rebuild_labels(&dir, status)?; let checks = self.performance_stats(); let builds = self.check_meta_queue_builds(&dir)?; diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index 071f328..4f7362b 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -9,7 +9,7 @@ use hubcaps::gists::Gists; use hubcaps::issues::Issue; use ofborg::acl::ACL; use ofborg::checkout; -use ofborg::commitstatus::CommitStatus; +use ofborg::commitstatus::{CommitStatus, CommitStatusError}; use ofborg::config::GithubAppVendingMachine; use ofborg::files::file_to_str; use ofborg::message::{buildjob, evaluationjob}; @@ -140,7 +140,6 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { let gists = client_legacy.gists(); let repo = client_app.repo(job.repo.owner.clone(), job.repo.name.clone()); - OneEval { client_app, repo, @@ -207,6 +206,22 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { eval_result.expect_err("We have an OK, but just checked for an Ok before"); match eval_result { + EvalWorkerError::CommitStatusWrite(e) => { + eprintln!( + "Failed to write commit status, got error: {:#?}, marking internal error", + e + ); + let issue_ref = self.repo.issue(self.job.pr.number); + update_labels(&issue_ref, &[String::from("ofborg-internal-error")], &[]); + } + EvalWorkerError::EvalError(eval::Error::CommitStatusWrite(e)) => { + eprintln!( + "Failed to write commit status, got error: {:#?}, marking internal error", + e + ); + let issue_ref = self.repo.issue(self.job.pr.number); + update_labels(&issue_ref, &[String::from("ofborg-internal-error")], &[]); + } EvalWorkerError::EvalError(eval::Error::Fail(msg)) => { self.update_status(msg, None, hubcaps::statuses::State::Failure) .expect("Failed to set status"); @@ -286,7 +301,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { None, ); - overall_status.set_with_description("Starting", hubcaps::statuses::State::Pending); + overall_status.set_with_description("Starting", hubcaps::statuses::State::Pending)?; evaluation_strategy.pre_clone()?; @@ -294,7 +309,8 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { .cloner .project(&job.repo.full_name, job.repo.clone_url.clone()); - overall_status.set_with_description("Cloning project", hubcaps::statuses::State::Pending); + overall_status + .set_with_description("Cloning project", hubcaps::statuses::State::Pending)?; info!("Working on {}", job.pr.number); let co = project @@ -309,7 +325,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { overall_status.set_with_description( format!("Checking out {}", &target_branch).as_ref(), hubcaps::statuses::State::Pending, - ); + )?; info!("Checking out target branch {}", &target_branch); let refpath = co.checkout_origin_ref(target_branch.as_ref()).unwrap(); @@ -324,13 +340,13 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { self.events .notify(Event::EvaluationDurationCount(target_branch)); - overall_status.set_with_description("Fetching PR", hubcaps::statuses::State::Pending); + overall_status.set_with_description("Fetching PR", hubcaps::statuses::State::Pending)?; co.fetch_pr(job.pr.number).unwrap(); if !co.commit_exists(job.pr.head_sha.as_ref()) { overall_status - .set_with_description("Commit not found", hubcaps::statuses::State::Error); + .set_with_description("Commit not found", hubcaps::statuses::State::Error)?; info!("Commit {} doesn't exist", job.pr.head_sha); return Ok(self.actions().skip(&job)); @@ -338,11 +354,11 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { evaluation_strategy.after_fetch(&co)?; - overall_status.set_with_description("Merging PR", hubcaps::statuses::State::Pending); + overall_status.set_with_description("Merging PR", hubcaps::statuses::State::Pending)?; if co.merge_commit(job.pr.head_sha.as_ref()).is_err() { overall_status - .set_with_description("Failed to merge", hubcaps::statuses::State::Failure); + .set_with_description("Failed to merge", hubcaps::statuses::State::Failure)?; info!("Failed to merge {}", job.pr.head_sha); @@ -355,7 +371,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { println!("Got path: {:?}, building", refpath); overall_status - .set_with_description("Beginning Evaluations", hubcaps::statuses::State::Pending); + .set_with_description("Beginning Evaluations", hubcaps::statuses::State::Pending)?; let eval_results: bool = evaluation_strategy .evaluation_checks() @@ -369,7 +385,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { None, ); - status.set(hubcaps::statuses::State::Pending); + status + .set(hubcaps::statuses::State::Pending) + .expect("Failed to set status on eval strategy"); let state: hubcaps::statuses::State; let gist_url: Option; @@ -389,7 +407,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { } status.set_url(gist_url); - status.set(state.clone()); + status + .set(state.clone()) + .expect("Failed to set status on eval strategy"); if state == hubcaps::statuses::State::Success { Ok(()) @@ -411,10 +431,10 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { info!("Just about done..."); - overall_status.set_with_description("^.^!", hubcaps::statuses::State::Success); + overall_status.set_with_description("^.^!", hubcaps::statuses::State::Success)?; } else { overall_status - .set_with_description("Complete, with errors", hubcaps::statuses::State::Failure); + .set_with_description("Complete, with errors", hubcaps::statuses::State::Failure)?; } self.events.notify(Event::TaskEvaluationCheckComplete); @@ -552,6 +572,7 @@ fn indicates_wip(text: &str) -> bool { enum EvalWorkerError { EvalError(eval::Error), + CommitStatusWrite(CommitStatusError), } impl From for EvalWorkerError { @@ -559,3 +580,9 @@ impl From for EvalWorkerError { EvalWorkerError::EvalError(e) } } + +impl From for EvalWorkerError { + fn from(e: CommitStatusError) -> EvalWorkerError { + EvalWorkerError::CommitStatusWrite(e) + } +}