Repository: osbuild/osbuild-composer
Pull request #1083 (2020-11-12 19:36:17)

workerapi: serialize koji errors as stringss
Serializing an interface does not work, let us simply use the string
representation and treat the empty string as no error. This is
compatible with the current API in the success case, and fixes the
error case, which is currently broken.

Also extend the test matrix for the kojiapi to ensure that all the
different kinds of errors can be serialized correctly and leads to
the correct status being returned.


Patch:
diff --git a/cmd/osbuild-worker/jobimpl-koji-finalize.go b/cmd/osbuild-worker/jobimpl-koji-finalize.go
index ae7099975..7479aabc9 100644
--- a/cmd/osbuild-worker/jobimpl-koji-finalize.go
+++ b/cmd/osbuild-worker/jobimpl-koji-finalize.go
@@ -105,7 +105,7 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error {
 		return err
 	}
 
-	if initArgs.KojiError != nil {
+	if initArgs.KojiError != "" {
 		failure = true
 	}
 
@@ -127,7 +127,7 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error {
 		if err != nil {
 			return err
 		}
-		if !buildArgs.OSBuildOutput.Success || buildArgs.KojiError != nil {
+		if !buildArgs.OSBuildOutput.Success || buildArgs.KojiError != "" {
 			failure = true
 			break
 		}
@@ -167,9 +167,15 @@ func (impl *KojiFinalizeJobImpl) Run(job worker.Job) error {
 
 	var result worker.KojiFinalizeJobResult
 	if failure {
-		result.KojiError = impl.kojiFail(args.Server, int(initArgs.BuildID), initArgs.Token)
+		err = impl.kojiFail(args.Server, int(initArgs.BuildID), initArgs.Token)
+		if err != nil {
+			result.KojiError = err.Error()
+		}
 	} else {
-		result.KojiError = impl.kojiImport(args.Server, build, buildRoots, images, args.KojiDirectory, initArgs.Token)
+		err = impl.kojiImport(args.Server, build, buildRoots, images, args.KojiDirectory, initArgs.Token)
+		if err != nil {
+			result.KojiError = err.Error()
+		}
 	}
 
 	err = job.Update(&result)
diff --git a/cmd/osbuild-worker/jobimpl-koji-init.go b/cmd/osbuild-worker/jobimpl-koji-init.go
index ca1970db1..21d54b506 100644
--- a/cmd/osbuild-worker/jobimpl-koji-init.go
+++ b/cmd/osbuild-worker/jobimpl-koji-init.go
@@ -60,7 +60,10 @@ func (impl *KojiInitJobImpl) Run(job worker.Job) error {
 	}
 
 	var result worker.KojiInitJobResult
-	result.Token, result.BuildID, result.KojiError = impl.kojiInit(args.Server, args.Name, args.Version, args.Release)
+	result.Token, result.BuildID, err = impl.kojiInit(args.Server, args.Name, args.Version, args.Release)
+	if err != nil {
+		result.KojiError = err.Error()
+	}
 
 	err = job.Update(&result)
 	if err != nil {
diff --git a/cmd/osbuild-worker/jobimpl-osbuild-koji.go b/cmd/osbuild-worker/jobimpl-osbuild-koji.go
index 0087a2ddc..f7d2d26fb 100644
--- a/cmd/osbuild-worker/jobimpl-osbuild-koji.go
+++ b/cmd/osbuild-worker/jobimpl-osbuild-koji.go
@@ -84,7 +84,7 @@ func (impl *OSBuildKojiJobImpl) Run(job worker.Job) error {
 		return err
 	}
 
-	if initArgs.KojiError == nil {
+	if initArgs.KojiError == "" {
 		result.OSBuildOutput, err = RunOSBuild(args.Manifest, impl.Store, outputDirectory, os.Stderr)
 		if err != nil {
 			return err
@@ -95,7 +95,10 @@ func (impl *OSBuildKojiJobImpl) Run(job worker.Job) error {
 			if err != nil {
 				return err
 			}
-			result.ImageHash, result.ImageSize, result.KojiError = impl.kojiUpload(f, args.KojiServer, args.KojiDirectory, args.KojiFilename)
+			result.ImageHash, result.ImageSize, err = impl.kojiUpload(f, args.KojiServer, args.KojiDirectory, args.KojiFilename)
+			if err != nil {
+				result.KojiError = err.Error()
+			}
 		}
 	}
 
diff --git a/internal/kojiapi/server.go b/internal/kojiapi/server.go
index 09e928bd1..76b6eb336 100644
--- a/internal/kojiapi/server.go
+++ b/internal/kojiapi/server.go
@@ -190,7 +190,7 @@ func (h *apiHandlers) PostCompose(ctx echo.Context) error {
 		}
 		time.Sleep(500 * time.Millisecond)
 	}
-	if initResult.KojiError != nil {
+	if initResult.KojiError != "" {
 		return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Could not initialize build with koji: %v", initResult.KojiError))
 	}
 
@@ -224,7 +224,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni
 		return "failure"
 	}
 
-	if initResult.KojiError != nil {
+	if initResult.KojiError != "" {
 		return "failure"
 	}
 
@@ -232,7 +232,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni
 		if buildResult.OSBuildOutput != nil && !buildResult.OSBuildOutput.Success {
 			return "failure"
 		}
-		if buildResult.KojiError != nil {
+		if buildResult.KojiError != "" {
 			return "failure"
 		}
 	}
@@ -241,7 +241,7 @@ func composeStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiIni
 		return "pending"
 	}
 
-	if result.KojiError == nil {
+	if result.KojiError == "" {
 		return "success"
 	}
 
@@ -253,7 +253,7 @@ func imageStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiInitJ
 		return "failure"
 	}
 
-	if initResult.KojiError != nil {
+	if initResult.KojiError != "" {
 		return "failure"
 	}
 
@@ -265,7 +265,7 @@ func imageStatusFromJobStatus(js *worker.JobStatus, initResult *worker.KojiInitJ
 		return "building"
 	}
 
-	if buildResult.OSBuildOutput != nil && buildResult.OSBuildOutput.Success && buildResult.KojiError == nil {
+	if buildResult.OSBuildOutput != nil && buildResult.OSBuildOutput.Success && buildResult.KojiError == "" {
 		return "success"
 	}
 
diff --git a/internal/worker/json.go b/internal/worker/json.go
index 8828f316b..708a26fcb 100644
--- a/internal/worker/json.go
+++ b/internal/worker/json.go
@@ -36,7 +36,7 @@ type KojiInitJob struct {
 type KojiInitJobResult struct {
 	BuildID   uint64 `json:"build_id"`
 	Token     string `json:"token"`
-	KojiError error  `json:"koji_error"`
+	KojiError string `json:"koji_error"`
 }
 
 type OSBuildKojiJob struct {
@@ -53,7 +53,7 @@ type OSBuildKojiJobResult struct {
 	OSBuildOutput *osbuild.Result `json:"osbuild_output"`
 	ImageHash     string          `json:"image_hash"`
 	ImageSize     uint64          `json:"image_size"`
-	KojiError     error           `json:"koji_error"`
+	KojiError     string          `json:"koji_error"`
 }
 
 type KojiFinalizeJob struct {
@@ -68,7 +68,7 @@ type KojiFinalizeJob struct {
 }
 
 type KojiFinalizeJobResult struct {
-	KojiError error `json:"koji_error"`
+	KojiError string `json:"koji_error"`
 }
 
 //


Test patch:
diff --git a/internal/kojiapi/server_test.go b/internal/kojiapi/server_test.go
index f240e9507..c57b44c01 100644
--- a/internal/kojiapi/server_test.go
+++ b/internal/kojiapi/server_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/osbuild/osbuild-composer/internal/kojiapi/api"
 	distro_mock "github.com/osbuild/osbuild-composer/internal/mocks/distro"
 	rpmmd_mock "github.com/osbuild/osbuild-composer/internal/mocks/rpmmd"
+	"github.com/osbuild/osbuild-composer/internal/osbuild"
 	"github.com/osbuild/osbuild-composer/internal/test"
 	"github.com/osbuild/osbuild-composer/internal/worker"
 	"github.com/stretchr/testify/require"
@@ -55,6 +56,10 @@ func TestStatus(t *testing.T) {
 	test.TestRoute(t, handler, false, "GET", "/api/composer-koji/v1/status", ``, http.StatusOK, `{"status":"OK"}`, "message")
 }
 
+type jobResult struct {
+	Result interface{} `json:"result"`
+}
+
 func TestCompose(t *testing.T) {
 	dir, err := ioutil.TempDir("", "osbuild-composer-test-kojiapi-")
 	if err != nil {
@@ -64,139 +69,282 @@ func TestCompose(t *testing.T) {
 
 	kojiServer, workerServer := newTestKojiServer(t, dir)
 	handler := kojiServer.Handler("/api/composer-koji/v1")
-	var wg sync.WaitGroup
-	wg.Add(1)
-
-	go func() {
-		token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-init"})
-		require.NoError(t, err)
-		require.Equal(t, "koji-init", jobType)
 
-		var initJob worker.KojiInitJob
-		err = json.Unmarshal(rawJob, &initJob)
-		require.NoError(t, err)
-		require.Equal(t, "koji.example.com", initJob.Server)
-		require.Equal(t, "foo", initJob.Name)
-		require.Equal(t, "1", initJob.Version)
-		require.Equal(t, "2", initJob.Release)
+	type kojiCase struct {
+		initResult       worker.KojiInitJobResult
+		buildResult      worker.OSBuildKojiJobResult
+		finalizeResult   worker.KojiFinalizeJobResult
+		composeReplyCode int
+		composeReply     string
+		composeStatus    string
+	}
 
-		test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
-		"result": {
-			"build_id": 42,
-			"token": "foobar"
-		}
-	}`, http.StatusOK, `{}`)
-		wg.Done()
-	}()
-
-	test.TestRoute(t, handler, false, "POST", "/api/composer-koji/v1/compose", `
-	{
-		"name":"foo",
-		"version":"1",
-		"release":"2",
-		"distribution":"fedora-30",
-		"image_requests": [
-			{
-				"architecture": "x86_64",
-				"image_type": "qcow2",
-				"repositories": [
+	var cases = []kojiCase{
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+			},
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "success"
+					},
+					{
+						"status": "success"
+					}
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "success"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				KojiError: "failure",
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+			},
+			composeReplyCode: http.StatusBadRequest,
+			composeReply:     `{"message":"Could not initialize build with koji: failure"}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "failure"
+					},
+					{
+						"status": "failure"
+					}
+				],
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: false,
+				},
+			},
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "failure"
+					},
 					{
-						"baseurl": "https://repo.example.com/"
+						"status": "success"
 					}
-				]
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+				KojiError: "failure",
+			},
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
+					{
+						"status": "failure"
+					},
+					{
+						"status": "success"
+					}
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+		{
+			initResult: worker.KojiInitJobResult{
+				BuildID: 42,
+				Token:   `"foobar"`,
+			},
+			buildResult: worker.OSBuildKojiJobResult{
+				Arch:      "x86_64",
+				HostOS:    "fedora-30",
+				ImageHash: "browns",
+				ImageSize: 42,
+				OSBuildOutput: &osbuild.Result{
+					Success: true,
+				},
+			},
+			finalizeResult: worker.KojiFinalizeJobResult{
+				KojiError: "failure",
 			},
-			{
-				"architecture": "x86_64",
-				"image_type": "qcow2",
-				"repositories": [
+			composeReplyCode: http.StatusCreated,
+			composeReply:     `{"koji_build_id":42}`,
+			composeStatus: `{
+				"image_statuses": [
 					{
-						"baseurl": "https://repo.example.com/"
+						"status": "success"
+					},
+					{
+						"status": "success"
 					}
-				]
+				],
+				"koji_build_id": 42,
+				"koji_task_id": 0,
+				"status": "failure"
+			}`,
+		},
+	}
+	for _, c := range cases {
+		var wg sync.WaitGroup
+		wg.Add(1)
+
+		go func(t *testing.T, result worker.KojiInitJobResult) {
+			token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-init"})
+			require.NoError(t, err)
+			require.Equal(t, "koji-init", jobType)
+
+			var initJob worker.KojiInitJob
+			err = json.Unmarshal(rawJob, &initJob)
+			require.NoError(t, err)
+			require.Equal(t, "koji.example.com", initJob.Server)
+			require.Equal(t, "foo", initJob.Name)
+			require.Equal(t, "1", initJob.Version)
+			require.Equal(t, "2", initJob.Release)
+
+			initJobResult, err := json.Marshal(&jobResult{Result: result})
+			require.NoError(t, err)
+			test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(initJobResult), http.StatusOK, `{}`)
+
+			wg.Done()
+		}(t, c.initResult)
+
+		test.TestRoute(t, handler, false, "POST", "/api/composer-koji/v1/compose", `
+		{
+			"name":"foo",
+			"version":"1",
+			"release":"2",
+			"distribution":"fedora-30",
+			"image_requests": [
+				{
+					"architecture": "x86_64",
+					"image_type": "qcow2",
+					"repositories": [
+						{
+							"baseurl": "https://repo.example.com/"
+						}
+					]
+				},
+				{
+					"architecture": "x86_64",
+					"image_type": "qcow2",
+					"repositories": [
+						{
+							"baseurl": "https://repo.example.com/"
+						}
+					]
+				}
+			],
+			"koji": {
+				"server": "koji.example.com"
 			}
-		],
-		"koji": {
-			"server": "koji.example.com"
-		}
-	}`, http.StatusCreated, `{"koji_build_id":42}`, "id")
-	wg.Wait()
-
-	token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
-	require.NoError(t, err)
-	require.Equal(t, "osbuild-koji", jobType)
+		}`, c.composeReplyCode, c.composeReply, "id")
+		wg.Wait()
 
-	var osbuildJob worker.OSBuildKojiJob
-	err = json.Unmarshal(rawJob, &osbuildJob)
-	require.NoError(t, err)
-	require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
-	require.Equal(t, "test.img", osbuildJob.ImageName)
-	require.NotEmpty(t, osbuildJob.KojiDirectory)
-
-	test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
-		"result": {
-			"arch": "x86_64",
-			"host_os": "fedora-30",
-			"image_hash": "browns",
-			"image_size": 42,
-			"osbuild_output": {
-				"success": true
-			}
-		}
-	}`, http.StatusOK, `{}`)
+		token, _, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
+		require.NoError(t, err)
+		require.Equal(t, "osbuild-koji", jobType)
 
-	token, _, jobType, rawJob, _, err = workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
-	require.NoError(t, err)
-	require.Equal(t, "osbuild-koji", jobType)
+		var osbuildJob worker.OSBuildKojiJob
+		err = json.Unmarshal(rawJob, &osbuildJob)
+		require.NoError(t, err)
+		require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
+		require.Equal(t, "test.img", osbuildJob.ImageName)
+		require.NotEmpty(t, osbuildJob.KojiDirectory)
 
-	err = json.Unmarshal(rawJob, &osbuildJob)
-	require.NoError(t, err)
-	require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
-	require.Equal(t, "test.img", osbuildJob.ImageName)
-	require.NotEmpty(t, osbuildJob.KojiDirectory)
-
-	test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
-		"result": {
-			"arch": "x86_64",
-			"host_os": "fedora-30",
-			"image_hash": "browns",
-			"image_size": 42,
-			"osbuild_output": {
-				"success": true
-			}
-		}
-	}`, http.StatusOK, `{}`)
+		buildJobResult, err := json.Marshal(&jobResult{Result: c.buildResult})
+		require.NoError(t, err)
+		test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(buildJobResult), http.StatusOK, `{}`)
 
-	token, finalizeID, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-finalize"})
-	require.NoError(t, err)
-	require.Equal(t, "koji-finalize", jobType)
+		token, _, jobType, rawJob, _, err = workerServer.RequestJob(context.Background(), "x86_64", []string{"osbuild-koji"})
+		require.NoError(t, err)
+		require.Equal(t, "osbuild-koji", jobType)
 
-	var kojiFinalizeJob worker.KojiFinalizeJob
-	err = json.Unmarshal(rawJob, &kojiFinalizeJob)
-	require.NoError(t, err)
-	require.Equal(t, "koji.example.com", kojiFinalizeJob.Server)
-	require.Equal(t, "1", kojiFinalizeJob.Version)
-	require.Equal(t, "2", kojiFinalizeJob.Release)
-	require.ElementsMatch(t, []string{"foo-1-2.x86_64.img", "foo-1-2.x86_64.img"}, kojiFinalizeJob.KojiFilenames)
-	require.NotEmpty(t, kojiFinalizeJob.KojiDirectory)
-
-	test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
-		"result": {
-		}
-	}`, http.StatusOK, `{}`)
-
-	test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/composer-koji/v1/compose/%v", finalizeID), ``, http.StatusOK, `{
-		"image_statuses": [
-			{
-				"status": "success"
-			},
-			{
-				"status": "success"
+		err = json.Unmarshal(rawJob, &osbuildJob)
+		require.NoError(t, err)
+		require.Equal(t, "koji.example.com", osbuildJob.KojiServer)
+		require.Equal(t, "test.img", osbuildJob.ImageName)
+		require.NotEmpty(t, osbuildJob.KojiDirectory)
+
+		test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), `{
+			"result": {
+				"arch": "x86_64",
+				"host_os": "fedora-30",
+				"image_hash": "browns",
+				"image_size": 42,
+				"osbuild_output": {
+					"success": true
+				}
 			}
-		],
-		"koji_build_id": 42,
-		"koji_task_id": 0,
-		"status": "success"
-	}`)
+		}`, http.StatusOK, `{}`)
+
+		token, finalizeID, jobType, rawJob, _, err := workerServer.RequestJob(context.Background(), "x86_64", []string{"koji-finalize"})
+		require.NoError(t, err)
+		require.Equal(t, "koji-finalize", jobType)
+
+		var kojiFinalizeJob worker.KojiFinalizeJob
+		err = json.Unmarshal(rawJob, &kojiFinalizeJob)
+		require.NoError(t, err)
+		require.Equal(t, "koji.example.com", kojiFinalizeJob.Server)
+		require.Equal(t, "1", kojiFinalizeJob.Version)
+		require.Equal(t, "2", kojiFinalizeJob.Release)
+		require.ElementsMatch(t, []string{"foo-1-2.x86_64.img", "foo-1-2.x86_64.img"}, kojiFinalizeJob.KojiFilenames)
+		require.NotEmpty(t, kojiFinalizeJob.KojiDirectory)
+
+		finalizeResult, err := json.Marshal(&jobResult{Result: c.finalizeResult})
+		require.NoError(t, err)
+		test.TestRoute(t, workerServer, false, "PATCH", fmt.Sprintf("/api/worker/v1/jobs/%v", token), string(finalizeResult), http.StatusOK, `{}`)
+
+		test.TestRoute(t, handler, false, "GET", fmt.Sprintf("/api/composer-koji/v1/compose/%v", finalizeID), ``, http.StatusOK, c.composeStatus)
+	}
 }
 
 func TestRequest(t *testing.T) {