[v2] jenkins: refactor HTTP boilerplate
diff mbox

Message ID 1466054662-31910-1-git-send-email-andrew.donnellan@au1.ibm.com
State Accepted
Delegated to: Russell Currey
Headers show

Commit Message

Andrew Donnellan June 16, 2016, 5:24 a.m. UTC
All our side-effect-free Jenkins API calls follow the same pattern of
creating a hyper Client, making a GET request to the relevant JSON API
endpoint, and decoding the returned JSON into a JSON Object (right now the
top-level data types are all Objects). Refactor this boilerplate into a new
method, get_api_json_object().

Additionally, rather than creating a new Client for every request, use the
Client instantiated in main(). This also means that Jenkins will use HTTP
proxy settings just like Patchwork.

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

---

Build tested only.

V1->V2:
	* Use the hyper Client from main() rather than creating a new
	  Client for every request
---
 src/jenkins.rs | 37 ++++++++++++++++---------------------
 src/main.rs    | 20 +++++++++++++-------
 2 files changed, 29 insertions(+), 28 deletions(-)

Comments

Russell Currey June 22, 2016, 3:26 a.m. UTC | #1
On Thu, 2016-06-16 at 15:24 +1000, Andrew Donnellan wrote:
> All our side-effect-free Jenkins API calls follow the same pattern of
> creating a hyper Client, making a GET request to the relevant JSON API
> endpoint, and decoding the returned JSON into a JSON Object (right now the
> top-level data types are all Objects). Refactor this boilerplate into a new
> method, get_api_json_object().
> 
> Additionally, rather than creating a new Client for every request, use the
> Client instantiated in main(). This also means that Jenkins will use HTTP
> proxy settings just like Patchwork.
> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> 
> ---

Merged to master as dbf9983e7358b59b267b97f948407e2babc92c64...6 days ago.
Didn't send an email or update Patchwork since I'm a terrible human being.

Patch
diff mbox

diff --git a/src/jenkins.rs b/src/jenkins.rs
index 8ce9c3c..88297e4 100644
--- a/src/jenkins.rs
+++ b/src/jenkins.rs
@@ -43,6 +43,7 @@  pub trait CIBackend { // TODO: Separate out
 
 pub struct JenkinsBackend<'a> {
     pub base_url: &'a str,
+    pub hyper_client: &'a Client,
     // TODO: Authentication
 }
 
@@ -54,12 +55,14 @@  impl<'a> CIBackend for JenkinsBackend<'a> {
     /// Returns Err when HTTP request fails or when no Location: header is returned
     fn start_test(&self, job_name: &str, params: Vec<(&str, &str)>)
                   -> Result<String, &'static str> {
-        let client = Client::new(); // TODO: do we want to get this from somewhere else?
         let params = url::form_urlencoded::Serializer::new(String::new())
             .extend_pairs(params)
             .finish();
 
-        let res = client.post(&format!("{}/job/{}/buildWithParameters?{}", self.base_url, job_name, params)).send().expect("HTTP request error"); // TODO don't panic here
+        let res = self.hyper_client
+            .post(&format!("{}/job/{}/buildWithParameters?{}",
+                           self.base_url, job_name, params))
+            .send().expect("HTTP request error"); // TODO don't panic here
 
         match res.headers.get::<Location>() {
             Some(loc) => Ok(loc.to_string()),
@@ -74,34 +77,26 @@  pub enum JenkinsBuildStatus {
 }
 
 impl<'a> JenkinsBackend<'a> {
-    pub fn get_build_url(&self, build_queue_entry: &str) -> Option<String> {
-        let client = Client::new(); // TODO
-        let url = format!("{}api/json", build_queue_entry);
-
-        let mut resp = client.get(&url).send().expect("HTTP request error"); // TODO don't panic here
+    fn get_api_json_object(&self, base_url: &str) -> rustc_serialize::json::Object {
+        // TODO: Don't panic on failure, fail more gracefully
+        let url = format!("{}api/json", base_url);
+        let mut resp = self.hyper_client.get(&url).send().expect("HTTP request error");
         let mut result_str = String::new();
         resp.read_to_string(&mut result_str)
             .unwrap_or_else(|err| panic!("Couldn't read from server: {}", err));
-        let json = Json::from_str(&result_str).unwrap();
-        let obj = json.as_object().unwrap();
-
-        match obj.get("executable") {
+        let json = Json::from_str(&result_str).unwrap_or_else(|err| panic!("Couldn't parse JSON from Jenkins: {}", err));
+        json.as_object().unwrap().clone()
+    }
+    
+    pub fn get_build_url(&self, build_queue_entry: &str) -> Option<String> {
+        match self.get_api_json_object(build_queue_entry).get("executable") {
             Some(exec) => Some(exec.as_object().unwrap().get("url").unwrap().as_string().unwrap().to_string()),
             None => None
         }
     }
 
     pub fn get_build_status(&self, build_url: &str) -> JenkinsBuildStatus {
-        let client = Client::new();
-        let url = format!("{}api/json", build_url);
-        let mut resp = client.get(&url).send().expect("HTTP request error");
-        let mut result_str = String::new();
-        resp.read_to_string(&mut result_str)
-            .unwrap_or_else(|err| panic!("Couldn't read from server: {}", err));
-        let json = Json::from_str(&result_str)
-            .unwrap_or_else(|err| panic!("Couldn't decode JSON: {}", err));
-
-        match json.as_object().unwrap().get("building").unwrap().as_boolean().unwrap() {
+        match self.get_api_json_object(build_url).get("building").unwrap().as_boolean().unwrap() {
             true => JenkinsBuildStatus::Running,
             false => JenkinsBuildStatus::Done,
         }
diff --git a/src/main.rs b/src/main.rs
index c34e165..9bb2064 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -89,9 +89,13 @@  struct Args {
     flag_project: String,
 }
 
-fn run_tests(settings: &Config, project: &Project, tag: &str, branch_name: &str) -> Vec<TestResult> {
+fn run_tests(settings: &Config, client: &Client, project: &Project, tag: &str,
+             branch_name: &str) -> Vec<TestResult> {
     let mut results: Vec<TestResult> = Vec::new();
-    let jenkins = JenkinsBackend { base_url: &settings.jenkins.url };
+    let jenkins = JenkinsBackend {
+        base_url: &settings.jenkins.url,
+        hyper_client: &client,
+    };
     let project = project.clone();
     for job_params in project.jobs.iter() {
         let job_name = job_params.get("job").unwrap();
@@ -131,7 +135,7 @@  fn run_tests(settings: &Config, project: &Project, tag: &str, branch_name: &str)
     results
 }
 
-fn test_patch(settings: &Config, project: &Project, path: &Path) -> Vec<TestResult> {
+fn test_patch(settings: &Config, client: &Arc<Client>, project: &Project, path: &Path) -> Vec<TestResult> {
     let repo = project.get_repo().unwrap();
     let mut results: Vec<TestResult> = Vec::new();
     if !path.is_file() {
@@ -207,12 +211,14 @@  fn test_patch(settings: &Config, project: &Project, path: &Path) -> Vec<TestResu
 
         let settings = settings.clone();
         let project = project.clone();
+        let client = client.clone();
         let settings_clone = settings.clone();
         let test_all_branches = project.test_all_branches.unwrap_or(true);
 
         // We've set up a remote branch, time to kick off tests
         let test = thread::Builder::new().name(tag.to_string()).spawn(move || {
-            return run_tests(&settings_clone, &project, &tag, &branch_name);
+            return run_tests(&settings_clone, &client, &project, &tag,
+                             &branch_name);
         }).unwrap();
         results.append(&mut test.join().unwrap());
 
@@ -281,7 +287,7 @@  fn main() {
         match settings.projects.get(&args.flag_project) {
             None => panic!("Couldn't find project {}", args.flag_project),
             Some(project) => {
-                test_patch(&settings, &project, &patch);
+                test_patch(&settings, &client, &project, &patch);
             }
         }
 
@@ -295,7 +301,7 @@  fn main() {
             None => panic!("Couldn't find project {}", &series.project.linkname),
             Some(project) => {
                 let patch = patchwork.get_patch(&series);
-                test_patch(&settings, &project, &patch);
+                test_patch(&settings, &client, &project, &patch);
             }
         }
 
@@ -324,7 +330,7 @@  fn main() {
                 },
                 Some(project) => {
                     let patch = patchwork.get_patch(&series);
-                    let results = test_patch(&settings, &project, &patch);
+                    let results = test_patch(&settings, &client, &project, &patch);
                     // Delete the temporary directory with the patch in it
                     fs::remove_dir_all(patch.parent().unwrap()).unwrap_or_else(
                         |err| error!("Couldn't delete temp directory: {}", err));