diff --git a/cmd/osbuild-worker/export_test.go b/cmd/osbuild-worker/export_test.go index bd6e314361..df64dcdfe1 100644 --- a/cmd/osbuild-worker/export_test.go +++ b/cmd/osbuild-worker/export_test.go @@ -1,3 +1,6 @@ package main -var WorkerClientErrorFrom = workerClientErrorFrom +var ( + WorkerClientErrorFrom = workerClientErrorFrom + MakeJobErrorFromOsbuildOutput = makeJobErrorFromOsbuildOutput +) diff --git a/cmd/osbuild-worker/jobimpl-depsolve.go b/cmd/osbuild-worker/jobimpl-depsolve.go index 4e721f35ef..e6eedf2f09 100644 --- a/cmd/osbuild-worker/jobimpl-depsolve.go +++ b/cmd/osbuild-worker/jobimpl-depsolve.go @@ -77,23 +77,31 @@ func workerClientErrorFrom(err error) (*clienterrors.Error, error) { switch e := err.(type) { case dnfjson.Error: // Error originates from dnf-json + reason := fmt.Sprintf("DNF error occurred: %s", e.Kind) + details := e.Reason switch e.Kind { case "DepsolveError": - return clienterrors.WorkerClientError(clienterrors.ErrorDNFDepsolveError, err.Error(), e.Reason), nil + return clienterrors.WorkerClientError(clienterrors.ErrorDNFDepsolveError, reason, details), nil case "MarkingErrors": - return clienterrors.WorkerClientError(clienterrors.ErrorDNFMarkingErrors, err.Error(), e.Reason), nil + return clienterrors.WorkerClientError(clienterrors.ErrorDNFMarkingErrors, reason, details), nil case "RepoError": - return clienterrors.WorkerClientError(clienterrors.ErrorDNFRepoError, err.Error(), e.Reason), nil + return clienterrors.WorkerClientError(clienterrors.ErrorDNFRepoError, reason, details), nil default: err := fmt.Errorf("Unhandled dnf-json error in depsolve job: %v", err) // This still has the kind/reason format but a kind that's returned // by dnf-json and not explicitly handled here. - return clienterrors.WorkerClientError(clienterrors.ErrorDNFOtherError, err.Error(), e.Reason), err + return clienterrors.WorkerClientError(clienterrors.ErrorDNFOtherError, reason, details), err } default: - err := fmt.Errorf("rpmmd error in depsolve job: %v", err) + reason := "rpmmd error in depsolve job" + details := "" + if err == nil { + err = fmt.Errorf("workerClientErrorFrom expected an error to be processed. Not nil") + } else { + details = err.Error() + } // Error originates from internal/rpmmd, not from dnf-json - return clienterrors.WorkerClientError(clienterrors.ErrorRPMMDError, err.Error(), nil), err + return clienterrors.WorkerClientError(clienterrors.ErrorRPMMDError, reason, details), err } } diff --git a/cmd/osbuild-worker/jobimpl-depsolve_test.go b/cmd/osbuild-worker/jobimpl-depsolve_test.go index 1f1c2ba047..e3dcfcdc48 100644 --- a/cmd/osbuild-worker/jobimpl-depsolve_test.go +++ b/cmd/osbuild-worker/jobimpl-depsolve_test.go @@ -18,8 +18,7 @@ func TestWorkerClientErrorFromDnfJson(t *testing.T) { } clientErr, err := worker.WorkerClientErrorFrom(dnfJsonErr) assert.NoError(t, err) - // XXX: this is duplicating the details, see https://github.com/osbuild/images/issues/727 - assert.Equal(t, clientErr.String(), `Code: 20, Reason: DNF error occurred: DepsolveError: something is terribly wrong, Details: something is terribly wrong`) + assert.Equal(t, `Code: 20, Reason: DNF error occurred: DepsolveError, Details: something is terribly wrong`, clientErr.String()) } func TestWorkerClientErrorFromOtherError(t *testing.T) { @@ -28,13 +27,12 @@ func TestWorkerClientErrorFromOtherError(t *testing.T) { // XXX: this is probably okay but it seems slightly dangerous to // assume that any "error" we get there is coming from rpmmd, can // we generate a more typed error from dnfjson here for rpmmd errors? - assert.EqualError(t, err, "rpmmd error in depsolve job: some error") - assert.Equal(t, clientErr.String(), `Code: 23, Reason: rpmmd error in depsolve job: some error, Details: `) + assert.EqualError(t, err, "some error") + assert.Equal(t, `Code: 23, Reason: rpmmd error in depsolve job, Details: some error`, clientErr.String()) } func TestWorkerClientErrorFromNil(t *testing.T) { clientErr, err := worker.WorkerClientErrorFrom(nil) - // XXX: this is wrong, it should generate an internal error - assert.EqualError(t, err, "rpmmd error in depsolve job: ") - assert.Equal(t, clientErr.String(), `Code: 23, Reason: rpmmd error in depsolve job: , Details: `) + assert.EqualError(t, err, "workerClientErrorFrom expected an error to be processed. Not nil") + assert.Equal(t, `Code: 23, Reason: rpmmd error in depsolve job, Details: `, clientErr.String()) } diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go index eca86fb5bf..54e2be8d82 100644 --- a/cmd/osbuild-worker/jobimpl-osbuild.go +++ b/cmd/osbuild-worker/jobimpl-osbuild.go @@ -351,6 +351,33 @@ func (impl *OSBuildJobImpl) getPulpClient(targetOptions *target.PulpOSTreeTarget return pulp.NewClientFromFile(address, impl.PulpConfig.CredsFilePath) } +func makeJobErrorFromOsbuildOutput(osbuildOutput *osbuild.Result) *clienterrors.Error { + var osbErrors []string + if osbuildOutput.Error != nil { + osbErrors = append(osbErrors, fmt.Sprintf("osbuild error: %s", string(osbuildOutput.Error))) + } + if osbuildOutput.Errors != nil { + for _, err := range osbuildOutput.Errors { + osbErrors = append(osbErrors, fmt.Sprintf("manifest validation error: %v", err)) + } + } + var failedStage string + for _, pipelineLog := range osbuildOutput.Log { + for _, stageResult := range pipelineLog { + if !stageResult.Success { + failedStage = stageResult.Type + break + } + } + } + + reason := "osbuild build failed" + if len(failedStage) > 0 { + reason += " in stage:\n" + failedStage + } + return clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, reason, osbErrors) +} + func (impl *OSBuildJobImpl) Run(job worker.Job) error { logWithId := logrus.WithField("jobId", job.Id().String()) // Initialize variable needed for reporting back to osbuild-composer. @@ -565,16 +592,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error { // Second handle the case when the build failed, but osbuild finished successfully if !osbuildJobResult.OSBuildOutput.Success { - var osbErrors []string - if osbuildJobResult.OSBuildOutput.Error != nil { - osbErrors = append(osbErrors, fmt.Sprintf("osbuild error: %s", string(osbuildJobResult.OSBuildOutput.Error))) - } - if osbuildJobResult.OSBuildOutput.Errors != nil { - for _, err := range osbuildJobResult.OSBuildOutput.Errors { - osbErrors = append(osbErrors, fmt.Sprintf("manifest validation error: %v", err)) - } - } - osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", osbErrors) + osbuildJobResult.JobError = makeJobErrorFromOsbuildOutput(osbuildJobResult.OSBuildOutput) return nil } diff --git a/cmd/osbuild-worker/jobimpl-osbuild_test.go b/cmd/osbuild-worker/jobimpl-osbuild_test.go new file mode 100644 index 0000000000..5e65570632 --- /dev/null +++ b/cmd/osbuild-worker/jobimpl-osbuild_test.go @@ -0,0 +1,81 @@ +package main_test + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/osbuild/images/pkg/osbuild" + + main "github.com/osbuild/osbuild-composer/cmd/osbuild-worker" +) + +func TestMakeJobErrorFromOsbuildOutput(t *testing.T) { + tests := []struct { + inputData *osbuild.Result + expected string + }{ + { + inputData: &osbuild.Result{ + Success: false, + Log: map[string]osbuild.PipelineResult{ + "fake-os": []osbuild.StageResult{ + { + Type: "good-stage", + Success: true, + Output: "good-output", + }, + { + Type: "bad-stage", + Success: false, + Output: "bad-failure", + }, + }, + }, + }, + expected: `Code: 10, Reason: osbuild build failed in stage: +bad-stage, Details: []`, + }, + { + inputData: &osbuild.Result{ + Success: false, + Log: map[string]osbuild.PipelineResult{ + "fake-os": []osbuild.StageResult{}, + }, + }, + expected: `Code: 10, Reason: osbuild build failed, Details: []`, + }, + { + inputData: &osbuild.Result{ + Error: json.RawMessage("some_osbuild_error"), + Success: false, + Log: map[string]osbuild.PipelineResult{ + "fake-os": []osbuild.StageResult{}, + }, + }, + expected: `Code: 10, Reason: osbuild build failed, Details: [osbuild error: some_osbuild_error]`, + }, + { + inputData: &osbuild.Result{ + Errors: []osbuild.ValidationError{ + { + Message: "validation error message", + Path: []string{"error path"}, + }, + }, + Success: false, + Log: map[string]osbuild.PipelineResult{ + "fake-os": []osbuild.StageResult{}, + }, + }, + expected: `Code: 10, Reason: osbuild build failed, Details: [manifest validation error: {validation error message [error path]}]`, + }, + } + for _, testData := range tests { + fakeOsbuildResult := testData.inputData + + wce := main.MakeJobErrorFromOsbuildOutput(fakeOsbuildResult) + require.Equal(t, testData.expected, wce.String()) + } +}