From f02c7382cc08664757d67ec591fbe0552750aa2e Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Mon, 6 Apr 2020 06:38:36 -0400 Subject: [PATCH 1/3] commitstatus: distinguish internal and external error factors --- ofborg/src/commitstatus.rs | 29 +++++++++++++++++++++++++++-- ofborg/src/tasks/evaluate.rs | 30 +++++++++++++++--------------- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/ofborg/src/commitstatus.rs b/ofborg/src/commitstatus.rs index cf92180..fb56db0 100644 --- a/ofborg/src/commitstatus.rs +++ b/ofborg/src/commitstatus.rs @@ -65,11 +65,36 @@ impl<'a> CommitStatus<'a> { .build(), ) .map(|_| ()) - .map_err(|e| CommitStatusError::HubcapsError(e)) + .map_err(|e| CommitStatusError::from(e)) } } #[derive(Debug)] pub enum CommitStatusError { - HubcapsError(hubcaps::Error), + MissingSHA(hubcaps::Error), + Error(hubcaps::Error), +} + +impl CommitStatusError { + pub fn is_internal_error(&self) -> bool { + match self { + CommitStatusError::MissingSHA(_) => false, + CommitStatusError::Error(_) => true, + } + } +} + +impl From for CommitStatusError { + fn from(e: hubcaps::Error) -> CommitStatusError { + use hyper::status::StatusCode; + match e.kind() { + hubcaps::ErrorKind::Fault { code, error } + if code == &StatusCode::UnprocessableEntity + && error.message.starts_with("No commit found for SHA:") => + { + CommitStatusError::MissingSHA(e) + } + _otherwise => CommitStatusError::Error(e), + } + } } diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index edd4c52..feb56d2 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -209,21 +209,21 @@ 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) => { - error!( - "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)) => { - error!( - "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::CommitStatusWrite(e) + | EvalWorkerError::EvalError(eval::Error::CommitStatusWrite(e)) => { + if e.is_internal_error() { + error!( + "Internal error writing commit status: {:?}, marking internal error", + e + ); + let issue_ref = self.repo.issue(self.job.pr.number); + update_labels(&issue_ref, &[String::from("ofborg-internal-error")], &[]); + } else { + error!( + "Ignorable error writing commit status: {:?}, marking internal error", + e + ); + } } EvalWorkerError::EvalError(eval::Error::Fail(msg)) => { self.update_status(msg.clone(), None, hubcaps::statuses::State::Failure) From 00478e9dee0db1e9906a12e0c92513e1d5d306a3 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Mon, 6 Apr 2020 06:40:53 -0400 Subject: [PATCH 2/3] update_status: also return a CommitStatusError --- ofborg/src/tasks/evaluate.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index feb56d2..bb0cc14 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -161,7 +161,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { description: String, url: Option, state: hubcaps::statuses::State, - ) -> Result<(), hubcaps::Error> { + ) -> Result<(), CommitStatusError> { let description = if description.len() >= 140 { warn!( "description is over 140 char; truncating: {:?}", @@ -189,6 +189,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { .statuses() .create(&self.job.pr.head_sha, &builder.build()) .map(|_| ()) + .map_err(|e| CommitStatusError::from(e)) } fn make_gist( From a1e8dcc1e536d48e6c8d105aab14029cf7c3ad69 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Mon, 6 Apr 2020 06:59:53 -0400 Subject: [PATCH 3/3] evaluate: handle evaluation errors, and posting errors while posting eval errors better --- ofborg/src/tasks/evaluate.rs | 75 +++++++++++++++++------------------- 1 file changed, 35 insertions(+), 40 deletions(-) diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index bb0cc14..e0d6c6a 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -202,52 +202,47 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> { } fn worker_actions(&mut self) -> worker::Actions { - let eval_result = self.evaluate_job(); - if let Ok(actions) = eval_result { - return actions; - } - let eval_result = - eval_result.expect_err("We have an OK, but just checked for an Ok before"); + 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, + self.make_gist(&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 { - EvalWorkerError::CommitStatusWrite(e) - | EvalWorkerError::EvalError(eval::Error::CommitStatusWrite(e)) => { - if e.is_internal_error() { - error!( - "Internal error writing commit status: {:?}, marking internal error", - e - ); - let issue_ref = self.repo.issue(self.job.pr.number); - update_labels(&issue_ref, &[String::from("ofborg-internal-error")], &[]); - } else { - error!( - "Ignorable error writing commit status: {:?}, marking internal error", - e - ); - } + Ok(eval_actions) => eval_actions, + Err(Ok(())) => { + // There was an error during eval, but we successfully + // updated the PR. + + self.actions().skip(&self.job) } - EvalWorkerError::EvalError(eval::Error::Fail(msg)) => { - self.update_status(msg.clone(), None, hubcaps::statuses::State::Failure) - .unwrap_or_else(|e| { - panic!("Failed to set plain status: {}; e: {:?}", msg, e); - }); + Err(Err(cswerr)) if !cswerr.is_internal_error() => { + error!("Ignorable error writing commit status: {:?}", cswerr); + + self.actions().skip(&self.job) } - EvalWorkerError::EvalError(eval::Error::FailWithGist(msg, filename, content)) => { - self.update_status( - msg.clone(), - self.make_gist(&filename, Some("".to_owned()), content.clone()), - hubcaps::statuses::State::Failure, - ) - .unwrap_or_else(|e| { - panic!( - "Failed to set status with a gist: {}, {}, {}; e: {:?}", - msg, filename, content, e - ); - }); + + Err(Err(cswerr)) => { + error!( + "Internal error writing commit status: {:?}, marking internal error", + cswerr + ); + let issue_ref = self.repo.issue(self.job.pr.number); + update_labels(&issue_ref, &[String::from("ofborg-internal-error")], &[]); + + self.actions().skip(&self.job) } } - - self.actions().skip(&self.job) } fn evaluate_job(&mut self) -> Result {