jenkins: Get started on fixing error handling
diff mbox series

Message ID 20181214043251.22323-1-andrew.donnellan@au1.ibm.com
State Accepted
Headers show
Series
  • jenkins: Get started on fixing error handling
Related show

Commit Message

Andrew Donnellan Dec. 14, 2018, 4:32 a.m. UTC
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 <andrew.donnellan@au1.ibm.com>

---

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(-)

Comments

Russell Currey Jan. 14, 2019, 5:32 a.m. UTC | #1
On Fri, 2018-12-14 at 15:32 +1100, Andrew Donnellan wrote:
> 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 <andrew.donnellan@au1.ibm.com>

Tested and applied as 05227dada827341197a1a6a8ccdcc6075c6adbb3, thanks

Patch
diff mbox series

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<Value, Box<Error>> {
         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<String> {
+    pub fn get_build_url(&self, build_queue_entry: &str) -> Result<String, Box<Error>> {
         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<BTreeMap>
-                            .unwrap() // BTreeMap
-                            .get("url") // Option<&str>
-                            .unwrap() // &str ?
-                            .as_str()
-                            .unwrap()
-                            .to_string(),
-                    );
+                    let url = exec
+                        .as_object() // Option<BTreeMap>
+                        .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<JenkinsBuildStatus, Box<Error>> {
+        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<TestState> {
+    pub fn get_build_result(&self, build_url: &str) -> Result<TestState, Box<Error>> {
         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<JenkinsBuildStatus, Box<Error>> {
         // 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<Error>> {
 }
 
 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<u8> = 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(()),
         }
     }
 }