From 89814c3107eb2a93f14156ec89e843043c5e41fe Mon Sep 17 00:00:00 2001
From: Lars Karlitski <lars@karlitski.net>
Date: Sun, 25 Oct 2020 08:45:10 +0100
Subject: [PATCH] worker: mark builds as failed based on osbuild's output

osbuild reports failing builds in two ways: it sets the "success" field
in its output to `false` and it returns with a non-zero exit status. The
worker used both, returning an `OSBuildError` when osbuild return
non-zero, but also forwarding the resulting object with the "success"
field.

Change this to only use the "success" field and ignore the return value.
The latter is useful for people running osbuild in a terminal or script,
but is redundant for this use-case.

This makes error reporting more consistent: `RunOSBuild` only returns an
error when *running* osbuild failed, not when the build fails.
---
 cmd/osbuild-worker/main.go    | 12 ++++++------
 cmd/osbuild-worker/osbuild.go | 20 ++++++++------------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/cmd/osbuild-worker/main.go b/cmd/osbuild-worker/main.go
index 0bd0f6305..bd57fb306 100644
--- a/cmd/osbuild-worker/main.go
+++ b/cmd/osbuild-worker/main.go
@@ -133,6 +133,11 @@ func RunJob(job worker.Job, store string, kojiServers map[string]koji.GSSAPICred
 
 	end_time := time.Now()
 
+	// Don't run uploaders if osbuild failed
+	if !result.Success {
+		return result, nil
+	}
+
 	var r []error
 
 	for _, t := range targets {
@@ -478,18 +483,13 @@ func main() {
 
 		var status common.ImageBuildState
 		result, err := RunJob(job, store, kojiServers)
-		if err != nil {
+		if err != nil || result.Success == false {
 			log.Printf("  Job failed: %v", err)
 			status = common.IBFailed
 
 			// Fail the jobs in any targets that expects it
 			FailJob(job, kojiServers)
 
-			// If the error comes from osbuild, retrieve the result
-			if osbuildError, ok := err.(*OSBuildError); ok {
-				result = osbuildError.Result
-			}
-
 			// Ensure we always have a non-nil result, composer doesn't like nils.
 			// This can happen in cases when OSBuild crashes and doesn't produce
 			// a meaningful output. E.g. when the machine runs of disk space.
diff --git a/cmd/osbuild-worker/osbuild.go b/cmd/osbuild-worker/osbuild.go
index d2c19914b..9059be49d 100644
--- a/cmd/osbuild-worker/osbuild.go
+++ b/cmd/osbuild-worker/osbuild.go
@@ -10,15 +10,11 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/osbuild"
 )
 
-type OSBuildError struct {
-	Message string
-	Result  *osbuild.Result
-}
-
-func (e *OSBuildError) Error() string {
-	return e.Message
-}
-
+// Run an instance of osbuild, returning a parsed osbuild.Result.
+//
+// Note that osbuild returns non-zero when the pipeline fails. This function
+// does not return an error in this case. Instead, the failure is communicated
+// with its corresponding logs through osbuild.Result.
 func RunOSBuild(manifest distro.Manifest, store, outputDirectory string, errorWriter io.Writer) (*osbuild.Result, error) {
 	cmd := exec.Command(
 		"osbuild",
@@ -58,9 +54,9 @@ func RunOSBuild(manifest distro.Manifest, store, outputDirectory string, errorWr
 
 	err = cmd.Wait()
 	if err != nil {
-		return nil, &OSBuildError{
-			Message: fmt.Sprintf("running osbuild failed: %v", err),
-			Result:  &result,
+		// ignore ExitError if output could be decoded correctly
+		if _, isExitError := err.(*exec.ExitError); !isExitError {
+			return nil, fmt.Errorf("running osbuild failed: %v", err)
 		}
 	}