buildresult: replace serialization with an enum to simplify compatibility

This separates the serialized BuildResult type that requires backwards
compatibility to avoid breaking results received from older builders and
the LegacyBuildResult type use internally.

ofborg/src/message/buildresult.rs

@@ -1,7 +1,7 @@
 use ofborg::message::{Pr, Repo};
 
 
-#[derive(Serialize, Deserialize, Debug)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)]
 pub enum BuildStatus {
     Skipped,
     Success,
@@ -10,16 +10,103 @@ pub enum BuildStatus {
     UnexpectedError { err: String },
 }
 
-#[derive(Serialize, Deserialize, Debug)]
+
-pub struct BuildResult {
+pub struct LegacyBuildResult {
     pub repo: Repo,
     pub pr: Pr,
     pub system: String,
     pub output: Vec<String>,
     pub attempt_id: String,
     pub request_id: String,
-    pub success: Option<bool>, // replaced by status
+    pub status: BuildStatus,
-    pub status: Option<BuildStatus>,
     pub skipped_attrs: Option<Vec<String>>,
     pub attempted_attrs: Option<Vec<String>>,
 }
+
+#[derive(Serialize, Deserialize, Debug)]
+#[serde(untagged)]
+pub enum BuildResult {
+    // TODO: introduce V1, V2, etc. and enable internal tagging.
+    Legacy {
+        repo: Repo,
+        pr: Pr,
+        system: String,
+        output: Vec<String>,
+        attempt_id: String,
+        request_id: Option<String>,
+        success: Option<bool>, // replaced by status
+        status: Option<BuildStatus>,
+        skipped_attrs: Option<Vec<String>>,
+        attempted_attrs: Option<Vec<String>>,
+    },
+}
+
+impl BuildResult {
+    pub fn legacy(&self) -> LegacyBuildResult {
+        // TODO: replace this with simpler structs for specific usecases, since
+        // it's decouples the structs from serialization.  These can be changed
+        // as long as we can translate all enum variants.
+        match self {
+            &BuildResult::Legacy { ref repo, ref pr, ref system, ref output, ref attempt_id, ref request_id, ref attempted_attrs, ref skipped_attrs, .. } =>
+                LegacyBuildResult {
+                    repo: repo.to_owned(),
+                    pr: pr.to_owned(),
+                    system: system.to_owned(),
+                    output: output.to_owned(),
+                    attempt_id: attempt_id.to_owned(),
+                    request_id: request_id.to_owned(),
+                    status: self.status(),
+                    attempted_attrs: attempted_attrs.to_owned(),
+                    skipped_attrs: skipped_attrs.to_owned(),
+                }
+        }
+    }
+
+    pub fn status(&self) -> BuildStatus {
+        match self {
+            &BuildResult::Legacy { ref status, ref success, .. } =>
+                status.to_owned().unwrap_or_else(|| {
+                    // Fallback for old format.
+                    match success {
+                        &None => BuildStatus::Skipped,
+                        &Some(true) => BuildStatus::Success,
+                        &Some(false) => BuildStatus::Failure,
+                    }
+                })
+        }
+    }
+}
+
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use serde_json;
+
+    #[test]
+    fn legacy_serialization() {
+        let input = r#"{"repo":{"owner":"NixOS","name":"nixpkgs","full_name":"NixOS/nixpkgs","clone_url":"https://github.com/nixos/nixpkgs.git"},"pr":{"target_branch":"master","number":42,"head_sha":"0000000000000000000000000000000000000000"},"system":"x86_64-linux","output":["unpacking sources"],"attempt_id":"attempt-id-foo","request_id":"bogus-request-id","success":true,"status":"Success","skipped_attrs":["AAAAAASomeThingsFailToEvaluate"],"attempted_attrs":["hello"]}"#;
+        let result: BuildResult = serde_json::from_str(input).expect("result required");
+        assert_eq!(result.status(), BuildStatus::Success);
+        let output = serde_json::to_string(&result).expect("json required");
+        assert_eq!(output, r#"{"repo":{"owner":"NixOS","name":"nixpkgs","full_name":"NixOS/nixpkgs","clone_url":"https://github.com/nixos/nixpkgs.git"},"pr":{"target_branch":"master","number":42,"head_sha":"0000000000000000000000000000000000000000"},"system":"x86_64-linux","output":["unpacking sources"],"attempt_id":"attempt-id-foo","request_id":"bogus-request-id","success":true,"status":"Success","skipped_attrs":["AAAAAASomeThingsFailToEvaluate"],"attempted_attrs":["hello"]}"#, "json of: {:?}", result);
+    }
+
+    #[test]
+    fn legacy_none_serialization() {
+        let input = r#"{"repo":{"owner":"NixOS","name":"nixpkgs","full_name":"NixOS/nixpkgs","clone_url":"https://github.com/nixos/nixpkgs.git"},"pr":{"target_branch":"master","number":42,"head_sha":"0000000000000000000000000000000000000000"},"system":"x86_64-linux","output":[],"attempt_id":"attempt-id-foo"}"#;
+        let result: BuildResult = serde_json::from_str(input).expect("result required");
+        assert_eq!(result.status(), BuildStatus::Skipped);
+        let output = serde_json::to_string(&result).expect("json required");
+        assert_eq!(output, r#"{"repo":{"owner":"NixOS","name":"nixpkgs","full_name":"NixOS/nixpkgs","clone_url":"https://github.com/nixos/nixpkgs.git"},"pr":{"target_branch":"master","number":42,"head_sha":"0000000000000000000000000000000000000000"},"system":"x86_64-linux","output":[],"attempt_id":"attempt-id-foo","request_id":null,"success":null,"status":null,"skipped_attrs":null,"attempted_attrs":null}"#, "json of: {:?}", result);
+    }
+
+    #[test]
+    fn legacy_no_status_serialization() {
+        let input = r#"{"repo":{"owner":"NixOS","name":"nixpkgs","full_name":"NixOS/nixpkgs","clone_url":"https://github.com/nixos/nixpkgs.git"},"pr":{"target_branch":"master","number":42,"head_sha":"0000000000000000000000000000000000000000"},"system":"x86_64-linux","output":["unpacking sources"],"attempt_id":"attempt-id-foo","request_id":"bogus-request-id","success":true,"status":null,"skipped_attrs":["AAAAAASomeThingsFailToEvaluate"],"attempted_attrs":["hello"]}"#;
+        let result: BuildResult = serde_json::from_str(input).expect("result required");
+        assert_eq!(result.status(), BuildStatus::Success);
+        let output = serde_json::to_string(&result).expect("json required");
+        assert_eq!(output, r#"{"repo":{"owner":"NixOS","name":"nixpkgs","full_name":"NixOS/nixpkgs","clone_url":"https://github.com/nixos/nixpkgs.git"},"pr":{"target_branch":"master","number":42,"head_sha":"0000000000000000000000000000000000000000"},"system":"x86_64-linux","output":["unpacking sources"],"attempt_id":"attempt-id-foo","request_id":"bogus-request-id","success":true,"status":null,"skipped_attrs":["AAAAAASomeThingsFailToEvaluate"],"attempted_attrs":["hello"]}"#, "json of: {:?}", result);
+    }
+}

ofborg/src/tasks/build.rs

@@ -112,7 +112,7 @@ impl<'a, 'b> JobActions<'a, 'b> {
     }
 
     pub fn merge_failed(&mut self) {
-        let msg = BuildResult {
+        let msg = BuildResult::Legacy {
             repo: self.job.repo.clone(),
             pr: self.job.pr.clone(),
             system: self.system.clone(),
@@ -196,7 +196,7 @@ impl<'a, 'b> JobActions<'a, 'b> {
     pub fn build_not_attempted(&mut self, not_attempted_attrs: Vec<String>,
 
     ) {
-        let msg = BuildResult {
+        let msg = BuildResult::Legacy {
             repo: self.job.repo.clone(),
             pr: self.job.pr.clone(),
             system: self.system.clone(),
@@ -237,7 +237,7 @@ impl<'a, 'b> JobActions<'a, 'b> {
             BuildStatus::Success => true,
             _ => false,
         };
-        let msg = BuildResult {
+        let msg = BuildResult::Legacy {
             repo: self.job.repo.clone(),
             pr: self.job.pr.clone(),
             system: self.system.clone(),

ofborg/src/tasks/githubcommentposter.rs

@@ -4,7 +4,7 @@ extern crate env_logger;
 use serde_json;
 
 use hubcaps;
-use ofborg::message::buildresult::{BuildStatus, BuildResult};
+use ofborg::message::buildresult::{BuildStatus, BuildResult, LegacyBuildResult};
 use ofborg::worker;
 use amqp::protocol::basic::{Deliver, BasicProperties};
 
@@ -41,28 +41,28 @@ impl worker::SimpleWorker for GitHubCommentPoster {
     }
 
     fn consumer(&mut self, job: &BuildResult) -> worker::Actions {
-        let comment = hubcaps::comments::CommentOptions { body: result_to_comment(&job) };
+        let result = job.legacy();
+        let comment = hubcaps::comments::CommentOptions { body: result_to_comment(&result) };
 
         let comment_attempt = self.github
-            .repo(job.repo.owner.clone(), job.repo.name.clone())
+            .repo(result.repo.owner.clone(), result.repo.name.clone())
             .pulls()
-            .get(job.pr.number)
+            .get(result.pr.number)
             .comments()
             .create(&comment);
 
         match comment_attempt {
             Ok(comment) => {
-                info!(
+                info!("Successfully sent {:?} to {}",
-                "Successfully sent {:?} to {}",
                 comment,
-                job.pr.number,
+                result.pr.number,
             )
             }
             Err(err) => {
                 info!(
                 "Failed to send comment {:?} to {}",
                 err,
-                job.pr.number,
+                result.pr.number,
             )
             }
         }
@@ -71,7 +71,7 @@ impl worker::SimpleWorker for GitHubCommentPoster {
     }
 }
 
-fn result_to_comment(result: &BuildResult) -> String {
+fn result_to_comment(result: &LegacyBuildResult) -> String {
     let mut reply: Vec<String> = vec![];
 
     let log_link = if result.output.len() > 0 {
@@ -86,21 +86,9 @@ fn result_to_comment(result: &BuildResult) -> String {
         "".to_owned()
     };
 
-    let status = match result.status {
-        None => {
-            // Fallback for old format.
-            match result.success {
-                None => &BuildStatus::Skipped,
-                Some(true) => &BuildStatus::Success,
-                Some(false) => &BuildStatus::Failure,
-            }
-        },
-        Some(ref s) => s,
-    };
-
     reply.push(format!("<!--REQUEST_ID={}-->", result.request_id));
     reply.push(format!("{} on {}{}",
-        (match *status {
+        (match result.status {
             BuildStatus::Skipped => "No attempt".into(),
             BuildStatus::Success => "Success".into(),
             BuildStatus::Failure => "Failure".into(),
@@ -170,7 +158,7 @@ mod tests {
 
     #[test]
     pub fn test_passing_build() {
-        let result = BuildResult {
+        let result = LegacyBuildResult {
             repo: Repo {
                 clone_url: "https://github.com/nixos/nixpkgs.git".to_owned(),
                 full_name: "NixOS/nixpkgs".to_owned(),
@@ -199,8 +187,7 @@ mod tests {
             system: "x86_64-linux".to_owned(),
             attempted_attrs: Some(vec!["foo".to_owned()]),
             skipped_attrs: Some(vec!["bar".to_owned()]),
-            status: Some(BuildStatus::Success),
+            status: BuildStatus::Success,
-            success: None,
         };
 
         assert_eq!(
@@ -234,7 +221,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
 
     #[test]
     pub fn test_failing_build() {
-        let result = BuildResult {
+        let result = LegacyBuildResult {
             repo: Repo {
                 clone_url: "https://github.com/nixos/nixpkgs.git".to_owned(),
                 full_name: "NixOS/nixpkgs".to_owned(),
@@ -263,8 +250,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
             system: "x86_64-linux".to_owned(),
             attempted_attrs: Some(vec!["foo".to_owned()]),
             skipped_attrs: None,
-            status: Some(BuildStatus::Failure),
+            status: BuildStatus::Failure,
-            success: None,
         };
 
         assert_eq!(
@@ -296,7 +282,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
 
     #[test]
     pub fn test_timedout_build() {
-        let result = BuildResult {
+        let result = LegacyBuildResult {
             repo: Repo {
                 clone_url: "https://github.com/nixos/nixpkgs.git".to_owned(),
                 full_name: "NixOS/nixpkgs".to_owned(),
@@ -324,8 +310,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
             system: "x86_64-linux".to_owned(),
             attempted_attrs: Some(vec!["foo".to_owned()]),
             skipped_attrs: None,
-            status: Some(BuildStatus::TimedOut),
+            status: BuildStatus::TimedOut,
-            success: None,
         };
 
         assert_eq!(
@@ -356,7 +341,7 @@ error: build of '/nix/store/l1limh50lx2cx45yb2gqpv7k8xl1mik2-gdb-8.1.drv' failed
 
     #[test]
     pub fn test_passing_build_unspecified_attributes() {
-        let result = BuildResult {
+        let result = LegacyBuildResult {
             repo: Repo {
                 clone_url: "https://github.com/nixos/nixpkgs.git".to_owned(),
                 full_name: "NixOS/nixpkgs".to_owned(),
@@ -385,8 +370,7 @@ error: build of '/nix/store/l1limh50lx2cx45yb2gqpv7k8xl1mik2-gdb-8.1.drv' failed
             system: "x86_64-linux".to_owned(),
             attempted_attrs: None,
             skipped_attrs: None,
-            status: Some(BuildStatus::Success),
+            status: BuildStatus::Success,
-            success: None,
         };
 
         assert_eq!(
@@ -416,7 +400,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
 
     #[test]
     pub fn test_failing_build_unspecified_attributes() {
-        let result = BuildResult {
+        let result = LegacyBuildResult {
             repo: Repo {
                 clone_url: "https://github.com/nixos/nixpkgs.git".to_owned(),
                 full_name: "NixOS/nixpkgs".to_owned(),
@@ -445,8 +429,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
             system: "x86_64-linux".to_owned(),
             attempted_attrs: None,
             skipped_attrs: None,
-            status: Some(BuildStatus::Failure),
+            status: BuildStatus::Failure,
-            success: None,
         };
 
         assert_eq!(
@@ -476,7 +459,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
 
     #[test]
     pub fn test_no_attempt() {
-        let result = BuildResult {
+        let result = LegacyBuildResult {
             repo: Repo {
                 clone_url: "https://github.com/nixos/nixpkgs.git".to_owned(),
                 full_name: "NixOS/nixpkgs".to_owned(),
@@ -494,8 +477,7 @@ patching script interpreter paths in /nix/store/pcja75y9isdvgz5i00pkrpif9rxzxc29
             system: "x86_64-linux".to_owned(),
             attempted_attrs: None,
             skipped_attrs: Some(vec!["not-attempted".to_owned()]),
-            status: Some(BuildStatus::Skipped),
+            status: BuildStatus::Skipped,
-            success: None,
         };
 
         assert_eq!(
@@ -518,7 +500,7 @@ foo
 
     #[test]
     pub fn test_no_attempt_no_log() {
-        let result = BuildResult {
+        let result = LegacyBuildResult {
             repo: Repo {
                 clone_url: "https://github.com/nixos/nixpkgs.git".to_owned(),
                 full_name: "NixOS/nixpkgs".to_owned(),
@@ -536,8 +518,7 @@ foo
             system: "x86_64-linux".to_owned(),
             attempted_attrs: None,
             skipped_attrs: Some(vec!["not-attempted".to_owned()]),
-            status: Some(BuildStatus::Skipped),
+            status: BuildStatus::Skipped,
-            success: None,
         };
 
         assert_eq!(

ofborg/src/tasks/log_message_collector.rs

@@ -204,7 +204,7 @@ impl worker::SimpleWorker for LogMessageCollector {
             } else {
                 let decode_msg: Result<BuildResult, _> = serde_json::from_slice(body);
                 if let Ok(msg) = decode_msg {
-                    attempt_id = msg.attempt_id.clone();
+                    attempt_id = msg.legacy().attempt_id.clone();
                     message = MsgType::Finish(msg);
                 } else {
                     return Err(format!("failed to decode job: {:?}", decode_msg));
@@ -430,7 +430,7 @@ mod tests {
                        worker.consumer(&
                                        LogMessage {
                                            from: make_from("foo"),
-                                           message: MsgType::Finish(BuildResult {
+                                           message: MsgType::Finish(BuildResult::Legacy {
                                                repo: Repo {
                                                    clone_url: "https://github.com/nixos/ofborg.git".to_owned(),
                                                    full_name: "NixOS/ofborg".to_owned(),