diff --git a/cmd/osbuild-worker/jobimpl-depsolve.go b/cmd/osbuild-worker/jobimpl-depsolve.go index e6eedf2f095..c88487f1e28 100644 --- a/cmd/osbuild-worker/jobimpl-depsolve.go +++ b/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.WorkerClientError(clienterrors.ErrorDNFDepsolveError, reason, details), nil + return clienterrors.WorkerClientError(clienterrors.ErrorDNFDepsolveError, reason, details) case "MarkingErrors": - return clienterrors.WorkerClientError(clienterrors.ErrorDNFMarkingErrors, reason, details), nil + return clienterrors.WorkerClientError(clienterrors.ErrorDNFMarkingErrors, reason, details) case "RepoError": - return clienterrors.WorkerClientError(clienterrors.ErrorDNFRepoError, reason, details), nil + return clienterrors.WorkerClientError(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.WorkerClientError(clienterrors.ErrorDNFOtherError, 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.WorkerClientError(clienterrors.ErrorDNFOtherError, reason, details) } default: 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() - } + details := fmt.Sprintf("%v", err) // Error originates from internal/rpmmd, not from dnf-json - return clienterrors.WorkerClientError(clienterrors.ErrorRPMMDError, reason, details), err + return clienterrors.WorkerClientError(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) - if err != nil { - logWithId.Errorf(err.Error()) - } + result.JobError = workerClientErrorFrom(err, logWithId) } if err := impl.Solver.CleanCache(); err != nil { // log and ignore diff --git a/cmd/osbuild-worker/jobimpl-depsolve_test.go b/cmd/osbuild-worker/jobimpl-depsolve_test.go index e3dcfcdc486..41e494fe709 100644 --- a/cmd/osbuild-worker/jobimpl-depsolve_test.go +++ b/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) - assert.NoError(t, err) + entry, hook := makeMockEntry() + 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) - // 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, "some error") + entry, hook := makeMockEntry() + clientErr := worker.WorkerClientErrorFrom(otherErr, entry) 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) - assert.EqualError(t, err, "workerClientErrorFrom expected an error to be processed. Not nil") + entry, hook := makeMockEntry() + clientErr := worker.WorkerClientErrorFrom(nil, entry) assert.Equal(t, `Code: 23, Reason: rpmmd error in depsolve job, Details: `, clientErr.String()) + assert.Equal(t, 1, len(hook.AllEntries())) + assert.Equal(t, "workerClientErrorFrom expected an error to be processed. Not nil", hook.LastEntry().Message) }