osbuild-worker: rework the workerClientErrorFrom() error

The workerClientErrorFrom() was returning an `*clienterrors.Error` and
an `error` (if something with the conversation goes wrong.

But the calling code was expecting that even if an `error` is returned
the `*clienterrors.Error` is still valid. The caller would then just
log the error. As returning a valid `value` even when there is an
`error` is an unexpected pattern this commit changes the code to
always return a `*clienterrors.Error` and log any issue via the
logger.

cmd/osbuild-worker/jobimpl-depsolve.go

@@ -73,7 +73,11 @@ func (impl *DepsolveJobImpl) depsolve(packageSets map[string][]rpmmd.PackageSet,
 	return depsolvedSets, repoConfigs, nil
 }
 
-func workerClientErrorFrom(err error) (*clienterrors.Error, error) {
+func workerClientErrorFrom(err error, logWithId *logrus.Entry) *clienterrors.Error {
+	if err == nil {
+		logWithId.Errorf("workerClientErrorFrom expected an error to be processed. Not nil")
+	}
+
 	switch e := err.(type) {
 	case dnfjson.Error:
 		// Error originates from dnf-json
@@ -81,27 +85,27 @@ func workerClientErrorFrom(err error) (*clienterrors.Error, error) {
 		details := e.Reason
 		switch e.Kind {
 		case "DepsolveError":
-			return clienterrors.New(clienterrors.ErrorDNFDepsolveError, reason, details), nil
+			return clienterrors.New(clienterrors.ErrorDNFDepsolveError, reason, details)
 		case "MarkingErrors":
-			return clienterrors.New(clienterrors.ErrorDNFMarkingErrors, reason, details), nil
+			return clienterrors.New(clienterrors.ErrorDNFMarkingErrors, reason, details)
 		case "RepoError":
-			return clienterrors.New(clienterrors.ErrorDNFRepoError, reason, details), nil
+			return clienterrors.New(clienterrors.ErrorDNFRepoError, reason, details)
 		default:
-			err := fmt.Errorf("Unhandled dnf-json error in depsolve job: %v", err)
+			logWithId.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.New(clienterrors.ErrorDNFOtherError, reason, details), err
+			return clienterrors.New(clienterrors.ErrorDNFOtherError, reason, details)
 		}
 	default:
 		reason := "rpmmd error in depsolve job"
-		details := "<nil>"
+		details := fmt.Sprintf("%v", err)
-		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.New(clienterrors.ErrorRPMMDError, reason, details), err
+		//
+		// XXX: 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?
+		return clienterrors.New(clienterrors.ErrorRPMMDError, reason, details)
 	}
 }
 
@@ -138,10 +142,7 @@ func (impl *DepsolveJobImpl) Run(job worker.Job) error {
 
 	result.PackageSpecs, result.RepoConfigs, err = impl.depsolve(args.PackageSets, args.ModulePlatformID, args.Arch, args.Releasever)
 	if err != nil {
-		result.JobError, err = workerClientErrorFrom(err)
+		result.JobError = workerClientErrorFrom(err, logWithId)
-		if err != nil {
-			logWithId.Errorf(err.Error())
-		}
 	}
 	if err := impl.Solver.CleanCache(); err != nil {
 		// log and ignore

cmd/osbuild-worker/jobimpl-depsolve_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/osbuild/images/pkg/dnfjson"
@@ -11,28 +13,46 @@ import (
 	worker "github.com/osbuild/osbuild-composer/cmd/osbuild-worker"
 )
 
+func makeMockEntry() (*logrus.Entry, *test.Hook) {
+	logger, hook := test.NewNullLogger()
+	return logger.WithField("test", "test"), hook
+}
+
 func TestWorkerClientErrorFromDnfJson(t *testing.T) {
 	dnfJsonErr := dnfjson.Error{
 		Kind:   "DepsolveError",
 		Reason: "something is terribly wrong",
 	}
-	clientErr, err := worker.WorkerClientErrorFrom(dnfJsonErr)
+	entry, hook := makeMockEntry()
-	assert.NoError(t, err)
+	clientErr := worker.WorkerClientErrorFrom(dnfJsonErr, entry)
 	assert.Equal(t, `Code: 20, Reason: DNF error occurred: DepsolveError, Details: something is terribly wrong`, clientErr.String())
+	assert.Equal(t, 0, len(hook.AllEntries()))
+}
+
+func TestWorkerClientErrorFromDnfJsonOtherKind(t *testing.T) {
+	dnfJsonErr := dnfjson.Error{
+		Kind:   "something-else",
+		Reason: "something is terribly wrong",
+	}
+	entry, hook := makeMockEntry()
+	clientErr := worker.WorkerClientErrorFrom(dnfJsonErr, entry)
+	assert.Equal(t, `Code: 22, Reason: DNF error occurred: something-else, Details: something is terribly wrong`, clientErr.String())
+	assert.Equal(t, 1, len(hook.AllEntries()))
+	assert.Equal(t, "Unhandled dnf-json error in depsolve job: DNF error occurred: something-else: something is terribly wrong", hook.LastEntry().Message)
 }
 
 func TestWorkerClientErrorFromOtherError(t *testing.T) {
 	otherErr := fmt.Errorf("some error")
-	clientErr, err := worker.WorkerClientErrorFrom(otherErr)
+	entry, hook := makeMockEntry()
-	// XXX: this is probably okay but it seems slightly dangerous to
+	clientErr := worker.WorkerClientErrorFrom(otherErr, entry)
-	// 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, "some error")
 	assert.Equal(t, `Code: 23, Reason: rpmmd error in depsolve job, Details: some error`, clientErr.String())
+	assert.Equal(t, 0, len(hook.AllEntries()))
 }
 
 func TestWorkerClientErrorFromNil(t *testing.T) {
-	clientErr, err := worker.WorkerClientErrorFrom(nil)
+	entry, hook := makeMockEntry()
-	assert.EqualError(t, err, "workerClientErrorFrom expected an error to be processed. Not nil")
+	clientErr := worker.WorkerClientErrorFrom(nil, entry)
 	assert.Equal(t, `Code: 23, Reason: rpmmd error in depsolve job, Details: <nil>`, clientErr.String())
+	assert.Equal(t, 1, len(hook.AllEntries()))
+	assert.Equal(t, "workerClientErrorFrom expected an error to be processed. Not nil", hook.LastEntry().Message)
 }