From patchwork Fri Dec 14 04:32:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Donnellan X-Patchwork-Id: 1013297 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43GHjW5wMHz9s4s for ; Fri, 14 Dec 2018 15:33:19 +1100 (AEDT) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=au1.ibm.com Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 43GHjW4SnTzDr7n for ; Fri, 14 Dec 2018 15:33:19 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=au1.ibm.com X-Original-To: snowpatch@lists.ozlabs.org Delivered-To: snowpatch@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=au1.ibm.com (client-ip=148.163.156.1; helo=mx0a-001b2d01.pphosted.com; envelope-from=andrew.donnellan@au1.ibm.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=au1.ibm.com Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 43GHjS1HgxzDr7b for ; Fri, 14 Dec 2018 15:33:15 +1100 (AEDT) Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id wBE4T0CR088416 for ; Thu, 13 Dec 2018 23:33:12 -0500 Received: from e06smtp02.uk.ibm.com (e06smtp02.uk.ibm.com [195.75.94.98]) by mx0a-001b2d01.pphosted.com with ESMTP id 2pc2p7e8vp-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 13 Dec 2018 23:33:12 -0500 Received: from localhost by e06smtp02.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 14 Dec 2018 04:33:08 -0000 Received: from b06cxnps3075.portsmouth.uk.ibm.com (9.149.109.195) by e06smtp02.uk.ibm.com (192.168.101.132) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Fri, 14 Dec 2018 04:33:06 -0000 Received: from d06av23.portsmouth.uk.ibm.com (d06av23.portsmouth.uk.ibm.com [9.149.105.59]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id wBE4X5dw54132968 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 14 Dec 2018 04:33:05 GMT Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2D8B9A4051; Fri, 14 Dec 2018 04:33:05 +0000 (GMT) Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 87EF0A404D; Fri, 14 Dec 2018 04:33:04 +0000 (GMT) Received: from ozlabs.au.ibm.com (unknown [9.192.253.14]) by d06av23.portsmouth.uk.ibm.com (Postfix) with ESMTP; Fri, 14 Dec 2018 04:33:04 +0000 (GMT) Received: from intelligence.ibm.com (unknown [9.102.53.94]) (using TLSv1.2 with cipher DHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.au.ibm.com (Postfix) with ESMTPSA id 21E54A0173; Fri, 14 Dec 2018 15:33:03 +1100 (AEDT) From: Andrew Donnellan To: snowpatch@lists.ozlabs.org Date: Fri, 14 Dec 2018 15:32:51 +1100 X-Mailer: git-send-email 2.11.0 X-TM-AS-GCONF: 00 x-cbid: 18121404-0008-0000-0000-000002A117B3 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18121404-0009-0000-0000-0000220B9BA5 Message-Id: <20181214043251.22323-1-andrew.donnellan@au1.ibm.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2018-12-14_02:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1812140040 Subject: [snowpatch] [PATCH] jenkins: Get started on fixing error handling X-BeenThere: snowpatch@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Continuous Integration for patch-based workflows List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: snowpatch-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "snowpatch" We use unwrap() a lot. We should do less of that. Get rid of a lot of the unwrap()s in the Jenkins code and return Result types with errors instead. We haven't completely gotten rid of unwrap(), there's still some bits which are a bit gross, and on the main side we still just unwrap rather than handling the error properly. At some point in the future, we might switch to using an external crate for the Jenkins API itself to make life a bit easier. Signed-off-by: Andrew Donnellan --- not very tested --- src/jenkins.rs | 72 ++++++++++++++++++++++++++++---------------------------- src/main.rs | 14 ++++++----- src/patchwork.rs | 13 +++++----- src/settings.rs | 2 +- 4 files changed, 51 insertions(+), 50 deletions(-) diff --git a/src/jenkins.rs b/src/jenkins.rs index 67d729e1bc94..593cee56716d 100644 --- a/src/jenkins.rs +++ b/src/jenkins.rs @@ -25,6 +25,7 @@ extern crate reqwest; extern crate url; use std::collections::BTreeMap; +use std::error::Error; use std::io::Read; use std::sync::Arc; use std::thread::sleep; @@ -73,7 +74,8 @@ impl CIBackend for JenkinsBackend { .post_url(&format!( "{}/job/{}/buildWithParameters?{}", self.base_url, job_name, params - )).expect("HTTP request error"); // TODO don't panic here + )) + .expect("HTTP request error"); // TODO don't panic here match resp.headers().get(LOCATION) { // TODO do we actually have to return a string, coud we change the API? @@ -115,14 +117,14 @@ impl JenkinsBackend { self.reqwest_client.post(url).headers(self.headers()).send() } - fn get_api_json_object(&self, base_url: &str) -> Value { - // TODO: Don't panic on failure, fail more gracefully + fn get_api_json_object(&self, base_url: &str) -> Result> { let url = format!("{}api/json", base_url); let mut result_str = String::new(); loop { let mut resp = match self.get_url(&url) { Ok(r) => r, Err(e) => { + // TODO: We have to time out rather than spinning indefinitely warn!("Couldn't hit Jenkins API: {}", e); sleep(Duration::from_millis(JENKINS_POLLING_INTERVAL)); continue; @@ -130,62 +132,60 @@ impl JenkinsBackend { }; if resp.status().is_server_error() { + // TODO: Timeout sleep(Duration::from_millis(JENKINS_POLLING_INTERVAL)); continue; } resp.read_to_string(&mut result_str) - .unwrap_or_else(|err| panic!("Couldn't read from server: {}", err)); + .map_err(|e| format!("Couldn't read from server: {}", e))?; break; } serde_json::from_str(&result_str) - .unwrap_or_else(|err| panic!("Couldn't parse JSON from Jenkins: {}", err)) + .map_err(|e| format!("Couldn't parse JSON from Jenkins: {}", e).into()) } - pub fn get_build_url(&self, build_queue_entry: &str) -> Option { + pub fn get_build_url(&self, build_queue_entry: &str) -> Result> { loop { - let entry = self.get_api_json_object(build_queue_entry); + let entry = self.get_api_json_object(build_queue_entry)?; match entry.get("executable") { Some(exec) => { - return Some( - exec.as_object() // Option - .unwrap() // BTreeMap - .get("url") // Option<&str> - .unwrap() // &str ? - .as_str() - .unwrap() - .to_string(), - ); + let url = exec + .as_object() // Option + .unwrap() // BTreeMap + .get("url") // Option<&str> + .unwrap() // &str ? + .as_str() + .unwrap() + .to_string(); + return Ok(url); } + // TODO: Timeout / handle this case properly None => sleep(Duration::from_millis(JENKINS_POLLING_INTERVAL)), } } } - pub fn get_build_status(&self, build_url: &str) -> JenkinsBuildStatus { - if self.get_api_json_object(build_url)["building"] - .as_bool() - .unwrap() - { - JenkinsBuildStatus::Running - } else { - JenkinsBuildStatus::Done + pub fn get_build_status(&self, build_url: &str) -> Result> { + match self.get_api_json_object(build_url)?["building"].as_bool() { + Some(true) => Ok(JenkinsBuildStatus::Running), + Some(false) => Ok(JenkinsBuildStatus::Done), + None => Err("Error getting build status".into()), } } - pub fn get_build_result(&self, build_url: &str) -> Option { + pub fn get_build_result(&self, build_url: &str) -> Result> { match self - .get_api_json_object(build_url) + .get_api_json_object(build_url)? .get("result") - .unwrap() - .as_str() + .map(|v| v.as_str().unwrap_or("PENDING")) { - None => None, + None => Ok(TestState::Pending), Some(result) => match result { // TODO: Improve this... - "SUCCESS" => Some(TestState::Success), - "FAILURE" => Some(TestState::Fail), - "UNSTABLE" => Some(TestState::Warning), - _ => Some(TestState::Pending), + "SUCCESS" => Ok(TestState::Success), + "FAILURE" => Ok(TestState::Fail), + "UNSTABLE" => Ok(TestState::Warning), + _ => Ok(TestState::Pending), }, } } @@ -227,11 +227,11 @@ impl JenkinsBackend { } } - pub fn wait_build(&self, build_url: &str) -> JenkinsBuildStatus { + pub fn wait_build(&self, build_url: &str) -> Result> { // TODO: Implement a timeout? - while self.get_build_status(build_url) != JenkinsBuildStatus::Done { + while self.get_build_status(build_url)? != JenkinsBuildStatus::Done { sleep(Duration::from_millis(JENKINS_POLLING_INTERVAL)); } - JenkinsBuildStatus::Done + Ok(JenkinsBuildStatus::Done) } } diff --git a/src/main.rs b/src/main.rs index 3849fcfee8c9..2ca2b007ecba 100644 --- a/src/main.rs +++ b/src/main.rs @@ -137,13 +137,13 @@ fn run_tests( let build_url_real; loop { let build_url = jenkins.get_build_url(&res); - if let Some(url) = build_url { + if let Ok(url) = build_url { build_url_real = url; break; - } + } // TODO: Handle error } debug!("Build URL: {}", build_url_real); - jenkins.wait_build(&build_url_real); + jenkins.wait_build(&build_url_real).unwrap(); // TODO: Error correctly let mut test_result = jenkins.get_build_result(&build_url_real).unwrap(); info!("Jenkins job for {}/{} complete.", branch_name, job.title); if test_result == TestState::Fail && job.warn_on_fail { @@ -231,7 +231,8 @@ fn test_patch( branch_name.to_string(), "apply_patch".to_string(), "Successfully applied".to_string() - ).to_string(), + ) + .to_string(), ), context: Some("apply_patch".to_string()), ..Default::default() @@ -247,7 +248,8 @@ fn test_patch( branch_name.to_string(), "apply_patch".to_string(), "Patch failed to apply".to_string() - ).to_string(), + ) + .to_string(), ), context: Some("apply_patch".to_string()), ..Default::default() @@ -502,7 +504,7 @@ fn run() -> Result<(), Box> { } fn main() { - if let Err(e) = run() { + if let Err(e) = run() { println!("Error: {}", e); process::exit(1); } diff --git a/src/patchwork.rs b/src/patchwork.rs index f7c908ff4049..1c2d3821300b 100644 --- a/src/patchwork.rs +++ b/src/patchwork.rs @@ -270,13 +270,12 @@ impl PatchworkServer { let encoded = serde_json::to_string(&result).unwrap(); let headers = self.headers.clone(); debug!("JSON Encoded: {}", encoded); - let mut resp = try!( - self.client - .post(checks_url) - .headers(headers) - .body(encoded) - .send() - ); + let mut resp = try!(self + .client + .post(checks_url) + .headers(headers) + .body(encoded) + .send()); let mut body: Vec = vec![]; io::copy(&mut resp, &mut body).unwrap(); trace!("{}", String::from_utf8(body).unwrap()); diff --git a/src/settings.rs b/src/settings.rs index 8e86b213a6bc..83097dde1e27 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -222,7 +222,7 @@ mod test { fn parse_example_invalid() -> Result<(), &'static str> { match parse("examples/tests/invalid.toml") { Ok(_) => Err("Didn't fail parsing invalid TOML?"), - Err(_) => Ok(()) + Err(_) => Ok(()), } } }