danderson/ofborg
0
0
Fork 0
forked from the-distro/ofborg
Code Pull requests Activity

commitstatus: handle errors intsead of expecting them (mostly)

Browse source
This commit is contained in:
Graham Christensen 2020-03-31 19:05:18 -04:00
parent 750bc88e5f
commit 90fda07cf2
No known key found for this signature in database
GPG key ID: FE918C3A98C1030F
3 changed files with 69 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
ofborg/src/tasks/eval/mod.rs
View file

@ -6,7 +6,7 @@ mod generic;
pub use self::generic::GenericStrategy; pub use self::generic::GenericStrategy;
use hubcaps::checks::CheckRunOptions; use hubcaps::checks::CheckRunOptions;
use ofborg::checkout::CachedProjectCo; use ofborg::checkout::CachedProjectCo;
use ofborg::commitstatus::CommitStatus; use ofborg::commitstatus::{CommitStatus, CommitStatusError};
use ofborg::evalchecker::EvalChecker; use ofborg::evalchecker::EvalChecker;
use ofborg::message::buildjob::BuildJob; use ofborg::message::buildjob::BuildJob;
use std::path::Path; use std::path::Path;
@ -36,6 +36,13 @@ pub struct EvaluationComplete {
#[derive(Debug)] #[derive(Debug)]
pub enum Error { pub enum Error {
CommitStatusWrite(CommitStatusError),
Fail(String), Fail(String),
FailWithGist(String, String, String), FailWithGist(String, String, String),
} }
impl From<CommitStatusError> for Error {
fn from(e: CommitStatusError) -> Error {
Error::CommitStatusWrite(e)
}
}

33
ofborg/src/tasks/eval/nixpkgs.rs
View file

@ -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 { if let Some(ref rebuildsniff) = self.outpath_diff {
let mut rebuild_tags = RebuildTagger::new(); let mut rebuild_tags = RebuildTagger::new();
if let Some(attrs) = rebuildsniff.calculate_rebuild() { if let Some(attrs) = rebuildsniff.calculate_rebuild() {
if !attrs.is_empty() { if !attrs.is_empty() {
overall_status.set_url(self.gist_changed_paths(&attrs)); 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); rebuild_tags.parse_attrs(attrs);
@ -225,6 +229,7 @@ impl<'a> NixpkgsStrategy<'a> {
&rebuild_tags.tags_to_remove(), &rebuild_tags.tags_to_remove(),
); );
} }
Ok(())
} }
fn gist_changed_paths(&self, attrs: &[PackageArch]) -> Option<String> { fn gist_changed_paths(&self, attrs: &[PackageArch]) -> Option<String> {
@ -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 let changed_attributes = attrs
.iter() .iter()
.map(|attr| attr.package.split('.').collect::<Vec<&str>>()) .map(|attr| attr.package.split('.').collect::<Vec<&str>>())
@ -271,7 +276,7 @@ impl<'a> NixpkgsStrategy<'a> {
String::from("matching changed paths to changed attrs..."), String::from("matching changed paths to changed attrs..."),
gist_url, gist_url,
); );
status.set(hubcaps::statuses::State::Success); status.set(hubcaps::statuses::State::Success)?;
if let Ok(ref maint) = m { if let Ok(ref maint) = m {
request_reviews(&maint, &self.pull); request_reviews(&maint, &self.pull);
@ -285,6 +290,8 @@ impl<'a> NixpkgsStrategy<'a> {
); );
} }
} }
Ok(())
} }
fn check_meta_queue_builds(&self, dir: &Path) -> StepResult<Vec<BuildJob>> { fn check_meta_queue_builds(&self, dir: &Path) -> StepResult<Vec<BuildJob>> {
@ -296,7 +303,7 @@ impl<'a> NixpkgsStrategy<'a> {
String::from("config.nix: checkMeta = true"), String::from("config.nix: checkMeta = true"),
None, 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); let nixenv = HydraNixEnv::new(self.nix.clone(), dir.to_path_buf(), true);
match nixenv.execute_with_stats() { match nixenv.execute_with_stats() {
@ -310,7 +317,7 @@ impl<'a> NixpkgsStrategy<'a> {
try_build.dedup(); try_build.dedup();
status.set_url(None); 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 { if !try_build.is_empty() && try_build.len() <= 10 {
// In the case of trying to merge master in to // In the case of trying to merge master in to
@ -332,7 +339,7 @@ impl<'a> NixpkgsStrategy<'a> {
} }
Err(out) => { Err(out) => {
status.set_url(make_gist(&self.gists, "Meta Check", None, out.display())); 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( Err(Error::Fail(String::from(
"Failed to validate package metadata.", "Failed to validate package metadata.",
))) )))
@ -354,13 +361,13 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> {
status.set_with_description( status.set_with_description(
"Checking original stdenvs", "Checking original stdenvs",
hubcaps::statuses::State::Pending, hubcaps::statuses::State::Pending,
); )?;
self.check_stdenvs_before(dir); self.check_stdenvs_before(dir);
status.set_with_description( status.set_with_description(
"Checking original out paths", "Checking original out paths",
hubcaps::statuses::State::Pending, hubcaps::statuses::State::Pending,
); )?;
self.check_outpaths_before(dir)?; self.check_outpaths_before(dir)?;
Ok(()) Ok(())
@ -396,10 +403,10 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> {
&["2.status: merge conflict".to_owned()], &["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(); 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()?; self.check_outpaths_after()?;
Ok(()) Ok(())
@ -535,10 +542,10 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> {
status.set_with_description( status.set_with_description(
"Calculating Changed Outputs", "Calculating Changed Outputs",
hubcaps::statuses::State::Pending, hubcaps::statuses::State::Pending,
); )?;
self.update_new_package_labels(); self.update_new_package_labels();
self.update_rebuild_labels(&dir, status); self.update_rebuild_labels(&dir, status)?;
let checks = self.performance_stats(); let checks = self.performance_stats();
let builds = self.check_meta_queue_builds(&dir)?; let builds = self.check_meta_queue_builds(&dir)?;

55
ofborg/src/tasks/evaluate.rs
View file

@ -9,7 +9,7 @@ use hubcaps::gists::Gists;
use hubcaps::issues::Issue; use hubcaps::issues::Issue;
use ofborg::acl::ACL; use ofborg::acl::ACL;
use ofborg::checkout; use ofborg::checkout;
use ofborg::commitstatus::CommitStatus; use ofborg::commitstatus::{CommitStatus, CommitStatusError};
use ofborg::config::GithubAppVendingMachine; use ofborg::config::GithubAppVendingMachine;
use ofborg::files::file_to_str; use ofborg::files::file_to_str;
use ofborg::message::{buildjob, evaluationjob}; use ofborg::message::{buildjob, evaluationjob};
@ -140,7 +140,6 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
let gists = client_legacy.gists(); let gists = client_legacy.gists();
let repo = client_app.repo(job.repo.owner.clone(), job.repo.name.clone()); let repo = client_app.repo(job.repo.owner.clone(), job.repo.name.clone());
OneEval { OneEval {
client_app, client_app,
repo, 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"); eval_result.expect_err("We have an OK, but just checked for an Ok before");
match eval_result { 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)) => { EvalWorkerError::EvalError(eval::Error::Fail(msg)) => {
self.update_status(msg, None, hubcaps::statuses::State::Failure) self.update_status(msg, None, hubcaps::statuses::State::Failure)
.expect("Failed to set status"); .expect("Failed to set status");
@ -286,7 +301,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
None, 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()?; evaluation_strategy.pre_clone()?;
@ -294,7 +309,8 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
.cloner .cloner
.project(&job.repo.full_name, job.repo.clone_url.clone()); .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); info!("Working on {}", job.pr.number);
let co = project let co = project
@ -309,7 +325,7 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
overall_status.set_with_description( overall_status.set_with_description(
format!("Checking out {}", &target_branch).as_ref(), format!("Checking out {}", &target_branch).as_ref(),
hubcaps::statuses::State::Pending, hubcaps::statuses::State::Pending,
); )?;
info!("Checking out target branch {}", &target_branch); info!("Checking out target branch {}", &target_branch);
let refpath = co.checkout_origin_ref(target_branch.as_ref()).unwrap(); 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 self.events
.notify(Event::EvaluationDurationCount(target_branch)); .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(); co.fetch_pr(job.pr.number).unwrap();
if !co.commit_exists(job.pr.head_sha.as_ref()) { if !co.commit_exists(job.pr.head_sha.as_ref()) {
overall_status 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); info!("Commit {} doesn't exist", job.pr.head_sha);
return Ok(self.actions().skip(&job)); return Ok(self.actions().skip(&job));
@ -338,11 +354,11 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
evaluation_strategy.after_fetch(&co)?; 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() { if co.merge_commit(job.pr.head_sha.as_ref()).is_err() {
overall_status 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); 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); println!("Got path: {:?}, building", refpath);
overall_status 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 let eval_results: bool = evaluation_strategy
.evaluation_checks() .evaluation_checks()
@ -369,7 +385,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
None, 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 state: hubcaps::statuses::State;
let gist_url: Option<String>; let gist_url: Option<String>;
@ -389,7 +407,9 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
} }
status.set_url(gist_url); 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 { if state == hubcaps::statuses::State::Success {
Ok(()) Ok(())
@ -411,10 +431,10 @@ impl<'a, E: stats::SysEvents + 'static> OneEval<'a, E> {
info!("Just about done..."); info!("Just about done...");
overall_status.set_with_description("^.^!", hubcaps::statuses::State::Success); overall_status.set_with_description("^.^!", hubcaps::statuses::State::Success)?;
} else { } else {
overall_status 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); self.events.notify(Event::TaskEvaluationCheckComplete);
@ -552,6 +572,7 @@ fn indicates_wip(text: &str) -> bool {
enum EvalWorkerError { enum EvalWorkerError {
EvalError(eval::Error), EvalError(eval::Error),
CommitStatusWrite(CommitStatusError),
} }
impl From<eval::Error> for EvalWorkerError { impl From<eval::Error> for EvalWorkerError {
@ -559,3 +580,9 @@ impl From<eval::Error> for EvalWorkerError {
EvalWorkerError::EvalError(e) EvalWorkerError::EvalError(e)
} }
} }
impl From<CommitStatusError> for EvalWorkerError {
fn from(e: CommitStatusError) -> EvalWorkerError {
EvalWorkerError::CommitStatusWrite(e)
}
}