Skip to content

refactor: replace go-multierror with errors.Join for native error aggregation #8791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tekton-robot merged 7 commits into from
Jun 11, 2025
Merged

refactor: replace go-multierror with errors.Join for native error aggregation #8791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6834cdd
refactor: remove go-multierror dependency and use errors.Join for err…
infernus01 May 21, 2025
f8b8169
chore: cleanup/tidy unused or redundant indirect dependencies
infernus01 May 21, 2025
306e790
Errors string should not be capitalized (recommended by go)
infernus01 Jun 4, 2025
7b36512
Remove dependency on go-multierror and update error expectations in t…
infernus01 Jun 4, 2025
9a2e73e
mark go-multierror as indirect via go mod tidy
infernus01 Jun 4, 2025
66f015b
Removed unnecessary commits
infernus01 Jun 4, 2025
3be56c3
Refactoring status.go and status_test.go files according to suggestions
infernus01 Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ require (
github.com/google/go-containerregistry v0.20.5
github.com/google/uuid v1.6.0
github.com/hashicorp/errwrap v1.1.0 // indirect
github.com/hashicorp/go-multierror v1.1.1
github.com/hashicorp/go-multierror v1.1.1 // indirect
github.com/hashicorp/golang-lru v1.0.2
github.com/jenkins-x/go-scm v1.14.58
github.com/mitchellh/go-homedir v1.1.0 // indirect
Expand Down
38 changes: 17 additions & 21 deletions pkg/pod/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ package pod
import (
"context"
"encoding/json"
"errors"
"fmt"
"strconv"
"strings"
"time"

"github.com/hashicorp/go-multierror"
"github.com/tektoncd/pipeline/internal/sidecarlogresults"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand Down Expand Up @@ -156,16 +156,12 @@ func MakeTaskRunStatus(ctx context.Context, logger *zap.SugaredLogger, tr v1.Tas
}
}

var merr *multierror.Error
if err := setTaskRunStatusBasedOnStepStatus(ctx, logger, stepStatuses, &tr, pod.Status.Phase, kubeclient, ts); err != nil {
merr = multierror.Append(merr, err)
}

err := setTaskRunStatusBasedOnStepStatus(ctx, logger, stepStatuses, &tr, pod.Status.Phase, kubeclient, ts)
setTaskRunStatusBasedOnSidecarStatus(sidecarStatuses, trs)

trs.Results = removeDuplicateResults(trs.Results)

return *trs, merr.ErrorOrNil()
return *trs, err
}

func createTaskResultsFromStepResults(stepRunRes []v1.TaskRunStepResult, neededStepResults map[string]string) []v1.TaskRunResult {
Expand Down Expand Up @@ -220,9 +216,9 @@ func getStepResultsFromSidecarLogs(sidecarLogResults []result.RunResult, contain
return stepResultsFromSidecarLogs, nil
}

func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredLogger, stepStatuses []corev1.ContainerStatus, tr *v1.TaskRun, podPhase corev1.PodPhase, kubeclient kubernetes.Interface, ts *v1.TaskSpec) *multierror.Error {
func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredLogger, stepStatuses []corev1.ContainerStatus, tr *v1.TaskRun, podPhase corev1.PodPhase, kubeclient kubernetes.Interface, ts *v1.TaskSpec) error {
trs := &tr.Status
var merr *multierror.Error
var errs []error
Copy link
Member

@aThorp96 aThorp96 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're joining them all at the end anyway, any reason we're not going something like this?

var errs error
slr, err := sidecarlogresults.GetResultsFromSidecarLogs(ctx, kubeclient, tr.Namespace, tr.Status.PodName, pipeline.ReservedResultsSidecarContainerName, podPhase)
errs = errors.Join(errs, err) // note that errors.Join handles err being nil internally :)
...
err = setTaskRunArtifactsFromRunResult(sidecarLogResults, &tras)
errs = errors.Join(errs, err)
return errs

list appending isn't free, so it seems like using errors.Join each time may be preferable, but maybe not. Not sure what the best practice is here, hence my asking.

Copy link
Member

@vdemeester vdemeester Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Looking at the implementation, errors.Join might not be the preferred way, as it loops through the errors multiple times.

Copy link
Member

@aThorp96 aThorp96 Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right. Amortized errors.Join is going to result in more allocations in addition to the extra iteration, since append isn't going to make a new array every call but make certainly definitely will...


// collect results from taskrun spec and taskspec
specResults := []v1.TaskResult{}
Expand All @@ -244,7 +240,7 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
if tr.Status.TaskSpec.Results != nil || artifactsSidecarCreated {
slr, err := sidecarlogresults.GetResultsFromSidecarLogs(ctx, kubeclient, tr.Namespace, tr.Status.PodName, pipeline.ReservedResultsSidecarContainerName, podPhase)
if err != nil {
merr = multierror.Append(merr, err)
errs = append(errs, err)
}
sidecarLogResults = append(sidecarLogResults, slr...)
}
Expand All @@ -258,7 +254,7 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
err := setTaskRunArtifactsFromRunResult(sidecarLogResults, &tras)
if err != nil {
logger.Errorf("Failed to set artifacts value from sidecar logs: %v", err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
} else {
trs.Artifacts = &tras
}
Expand All @@ -282,13 +278,13 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
// Identify StepResults needed by the Task Results
neededStepResults, err := findStepResultsFetchedByTask(s.Name, specResults)
if err != nil {
merr = multierror.Append(merr, err)
errs = append(errs, err)
}

// populate step results from sidecar logs
stepResultsFromSidecarLogs, err := getStepResultsFromSidecarLogs(sidecarLogResults, s.Name)
if err != nil {
merr = multierror.Append(merr, err)
errs = append(errs, err)
}
_, stepRunRes, _ := filterResults(stepResultsFromSidecarLogs, specResults, stepResults)
if tr.IsDone() {
Expand All @@ -301,7 +297,7 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
err = setStepArtifactsValueFromSidecarLogResult(sidecarLogResults, s.Name, &sas)
if err != nil {
logger.Errorf("Failed to set artifacts value from sidecar logs: %v", err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
}

// Parse termination messages
Expand All @@ -312,22 +308,22 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
results, err := termination.ParseMessage(logger, msg)
if err != nil {
logger.Errorf("termination message could not be parsed sas JSON: %v", err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
} else {
err := setStepArtifactsValueFromTerminationMessageRunResult(results, &sas)
if err != nil {
logger.Errorf("error setting step artifacts of step %q in taskrun %q: %v", s.Name, tr.Name, err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
}
time, err := extractStartedAtTimeFromResults(results)
if err != nil {
logger.Errorf("error setting the start time of step %q in taskrun %q: %v", s.Name, tr.Name, err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
}
exitCode, err := extractExitCodeFromResults(results)
if err != nil {
logger.Errorf("error extracting the exit code of step %q in taskrun %q: %v", s.Name, tr.Name, err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
}

taskResults, stepRunRes, filteredResults := filterResults(results, specResults, stepResults)
Expand All @@ -341,15 +337,15 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
err := setTaskRunArtifactsFromRunResult(filteredResults, &tras)
if err != nil {
logger.Errorf("error setting step artifacts in taskrun %q: %v", tr.Name, err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
}
trs.Artifacts.Merge(&tras)
trs.Artifacts.Merge(&sas)
}
msg, err = createMessageFromResults(filteredResults)
if err != nil {
logger.Errorf("%v", err)
merr = multierror.Append(merr, err)
errs = append(errs, err)
} else {
state.Terminated.Message = msg
}
Expand Down Expand Up @@ -388,7 +384,7 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
}
}

return merr
return errors.Join(errs...)
}

func setStepArtifactsValueFromSidecarLogResult(results []result.RunResult, name string, artifacts *v1.Artifacts) error {
Expand Down
17 changes: 8 additions & 9 deletions pkg/pod/status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/hashicorp/go-multierror"
"github.com/tektoncd/pipeline/internal/sidecarlogresults"
"github.com/tektoncd/pipeline/pkg/apis/config"
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
Expand Down Expand Up @@ -105,9 +104,9 @@ func TestSetTaskRunStatusBasedOnStepStatus(t *testing.T) {
for _, cs := range c.ContainerStatuses {
originalStatuses = append(originalStatuses, *cs.DeepCopy())
}
merr := setTaskRunStatusBasedOnStepStatus(context.Background(), logger, c.ContainerStatuses, &tr, corev1.PodRunning, kubeclient, &v1.TaskSpec{})
if merr != nil {
t.Errorf("setTaskRunStatusBasedOnStepStatus: %s", merr)
gotErr := setTaskRunStatusBasedOnStepStatus(t.Context(), logger, c.ContainerStatuses, &tr, corev1.PodRunning, kubeclient, &v1.TaskSpec{})
if gotErr != nil {
t.Errorf("setTaskRunStatusBasedOnStepStatus: %s", gotErr)
}
if d := cmp.Diff(originalStatuses, c.ContainerStatuses); d != "" {
t.Errorf("container statuses changed: %s", diff.PrintWantGot(d))
Expand Down Expand Up @@ -238,11 +237,11 @@ func TestSetTaskRunStatusBasedOnStepStatus_sidecar_logs(t *testing.T) {
ctx := config.ToContext(context.Background(), &config.Config{
FeatureFlags: featureFlags,
})
var wantErr *multierror.Error
wantErr = multierror.Append(wantErr, c.wantErr)
merr := setTaskRunStatusBasedOnStepStatus(ctx, logger, []corev1.ContainerStatus{{}}, &c.tr, pod.Status.Phase, kubeclient, ts)

if d := cmp.Diff(wantErr.Error(), merr.Error()); d != "" {
gotErr := setTaskRunStatusBasedOnStepStatus(ctx, logger, []corev1.ContainerStatus{{}}, &c.tr, pod.Status.Phase, kubeclient, ts)
if gotErr == nil {
t.Fatalf("Expected error but got nil")
}
if d := cmp.Diff(c.wantErr.Error(), gotErr.Error()); d != "" {
t.Errorf("Got unexpected error %s", diff.PrintWantGot(d))
}
})
Expand Down
9 changes: 4 additions & 5 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"strings"
"time"

"github.com/hashicorp/go-multierror"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
pipelineErrors "github.com/tektoncd/pipeline/pkg/apis/pipeline/errors"
Expand Down Expand Up @@ -326,11 +325,11 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, pr *v1
events.EmitError(controller.GetEventRecorder(ctx), err, pr)
}

merr := multierror.Append(previousError, err).ErrorOrNil()
errs := errors.Join(previousError, err)
if controller.IsPermanentError(previousError) {
return controller.NewPermanentError(merr)
return controller.NewPermanentError(errs)
}
return merr
return errs
}

// resolvePipelineState will attempt to resolve each referenced task in the pipeline's spec and all of the resources
Expand Down Expand Up @@ -1536,7 +1535,7 @@ func validateChildObjectsInPipelineRunStatus(ctx context.Context, prs v1.Pipelin
case taskRun, customRun:
continue
default:
err = multierror.Append(err, fmt.Errorf("child with name %s has unknown kind %s", cr.Name, cr.Kind))
err = errors.Join(err, fmt.Errorf("child with name %s has unknown kind %s", cr.Name, cr.Kind))
}
}

Expand Down
10 changes: 5 additions & 5 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,7 @@ spec:
prt := newPipelineRunTest(t, d)
defer prt.Cancel()

wantEvents := append(tc.wantEvents, "Warning InternalError 1 error occurred") //nolint:gocritic
wantEvents := append(tc.wantEvents, "Warning InternalError") //nolint:gocritic
reconciledRun, _ := prt.reconcileRun("foo", tc.pipelineRun.Name, wantEvents, tc.permanentError)

if reconciledRun.Status.CompletionTime == nil {
Expand Down Expand Up @@ -3555,7 +3555,7 @@ spec:
// See https://github.com/tektoncd/pipeline/issues/2647 for further details.
wantEvents := []string{
"Normal PipelineRunCouldntCancel PipelineRun \"test-pipeline-fails-to-cancel\" was cancelled but had errors trying to cancel TaskRuns",
"Warning InternalError 1 error occurred",
"Warning InternalError",
}
err = k8sevent.CheckEventsOrdered(t, testAssets.Recorder.Events, prName, wantEvents)
if err != nil {
Expand Down Expand Up @@ -3673,7 +3673,7 @@ spec:
// See https://github.com/tektoncd/pipeline/issues/2647 for further details.
wantEvents := []string{
"Normal PipelineRunCouldntTimeOut PipelineRun \"test-pipeline-fails-to-timeout\" was timed out but had errors trying to time out TaskRuns and/or Runs",
"Warning InternalError 1 error occurred",
"Warning InternalError",
}
err = k8sevent.CheckEventsOrdered(t, testAssets.Recorder.Events, prName, wantEvents)
if err != nil {
Expand Down Expand Up @@ -13947,7 +13947,7 @@ spec:
wantEvents := []string{
"Normal Started",
"Warning Failed [User error] PipelineRun foo/pr can't be Run; couldn't resolve all references: array Result Index 3 for Task pt-with-result Result platforms is out of bound of size 3",
"Warning InternalError 1 error occurred:",
"Warning InternalError",
}
prt := newPipelineRunTest(t, d)
defer prt.Cancel()
Expand Down Expand Up @@ -16939,7 +16939,7 @@ spec:
"Normal Started ",
"Warning TaskRunsCreationFailed Failed to create TaskRuns [\"test-pipeline-run-with-create-run-failed-hello-world\"]: expected workspace \"source\" to be provided by pipelinerun for pipeline task \"hello-world\"",
"Warning Failed expected workspace \"source\" to be provided by pipelinerun for pipeline task \"hello-world\"",
"Warning InternalError 1 error occurred:\n\t* error creating TaskRuns called [test-pipeline-run-with-create-run-failed-hello-world] for PipelineTask hello-world from PipelineRun test-pipeline-run-with-create-run-failed: expected workspace \"source\" to be provided by pipelinerun for pipeline task \"hello-world\"\n\n",
"Warning InternalError error creating TaskRuns called [test-pipeline-run-with-create-run-failed-hello-world] for PipelineTask hello-world from PipelineRun test-pipeline-run-with-create-run-failed: expected workspace \"source\" to be provided by pipelinerun for pipeline task \"hello-world\"",
}
reconciledRun, _ := prt.reconcileRun("foo", "test-pipeline-run-with-create-run-failed", wantEvents, true /* permanentError */)

Expand Down
13 changes: 6 additions & 7 deletions pkg/reconciler/taskrun/taskrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"strings"
"time"

"github.com/hashicorp/go-multierror"
"github.com/tektoncd/pipeline/internal/sidecarlogresults"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand Down Expand Up @@ -370,7 +369,7 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, tr *v1
// Send k8s events and cloud events (when configured)
events.Emit(ctx, beforeCondition, afterCondition, tr)

merr := multierror.Append(previousError).ErrorOrNil()
errs := []error{previousError}

// If the Run has been completed before and remains so at present,
// no need to update the labels and annotations
Expand All @@ -380,14 +379,14 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, tr *v1
if err != nil {
logger.Warn("Failed to update TaskRun labels/annotations", zap.Error(err))
events.EmitError(controller.GetEventRecorder(ctx), err, tr)
errs = append(errs, err)
}
merr = multierror.Append(merr, err).ErrorOrNil()
}

joinedErr := errors.Join(errs...)
if controller.IsPermanentError(previousError) {
return controller.NewPermanentError(merr)
return controller.NewPermanentError(joinedErr)
}
return merr
return joinedErr
}

// `prepare` fetches resources the taskrun depends on, runs validation and conversion
Expand Down Expand Up @@ -619,7 +618,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1.TaskRun, rtr *resourc
if err := c.pvcHandler.CreatePVCFromVolumeClaimTemplate(ctx, ws, *kmeta.NewControllerRef(tr), tr.Namespace); err != nil {
logger.Errorf("Failed to create PVC for TaskRun %s: %v", tr.Name, err)
tr.Status.MarkResourceFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
fmt.Errorf("Failed to create PVC for TaskRun %s workspaces correctly: %w",
fmt.Errorf("failed to create PVC for TaskRun %s workspaces correctly: %w",
fmt.Sprintf("%s/%s", tr.Namespace, tr.Name), err))
return controller.NewPermanentError(err)
}
Expand Down
10 changes: 5 additions & 5 deletions pkg/reconciler/taskrun/taskrun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1627,7 +1627,7 @@ status:
startTime: "2021-12-31T23:59:59Z"
completionTime: "2022-01-01T00:00:00Z"
`)
prepareError = errors.New("1 error occurred:\n\t* error when listing tasks for taskRun test-taskrun-run-retry-prepare-failure: tasks.tekton.dev \"test-task\" not found")
prepareError = errors.New("error when listing tasks for taskRun test-taskrun-run-retry-prepare-failure: tasks.tekton.dev \"test-task\" not found")
toFailOnReconcileFailureTaskRun = parse.MustParseV1TaskRun(t, `
metadata:
name: test-taskrun-results-type-mismatched
Expand Down Expand Up @@ -1692,7 +1692,7 @@ status:
coschedule: "workspaces"
disableInlineSpec: ""
`, pipelineErrors.UserErrorLabel, pipelineErrors.UserErrorLabel))
reconciliatonError = errors.New("1 error occurred:\n\t* Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\"")
reconciliatonError = errors.New("Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\"")
toBeRetriedTaskRun = parse.MustParseV1TaskRun(t, `
metadata:
name: test-taskrun-to-be-retried
Expand Down Expand Up @@ -6218,7 +6218,7 @@ spec:
}},
}

expectedErr := errors.New("1 error occurred:\n\t* param `param1` value: invalid is not in the enum list")
expectedErr := errors.New("param `param1` value: invalid is not in the enum list")
expectedFailureReason := "InvalidParamValue"
testAssets, cancel := getTaskRunController(t, d)
defer cancel()
Expand Down Expand Up @@ -6380,13 +6380,13 @@ status:
name: "taskrun results type mismatched",
taskRun: taskRunResultsTypeMismatched,
wantFailedReason: v1.TaskRunReasonFailedValidation.String(),
expectedError: errors.New("1 error occurred:\n\t* Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\", \"objectResult\": task result is expected to be \"object\" type but was initialized to a different type \"string\""),
expectedError: errors.New("Provided results don't match declared results; may be invalid JSON or missing result declaration: \"aResult\": task result is expected to be \"array\" type but was initialized to a different type \"string\", \"objectResult\": task result is expected to be \"object\" type but was initialized to a different type \"string\""),
expectedResults: nil,
}, {
name: "taskrun results object miss key",
taskRun: taskRunResultsObjectMissKey,
wantFailedReason: v1.TaskRunReasonFailedValidation.String(),
expectedError: errors.New("1 error occurred:\n\t* missing keys for these results which are required in TaskResult's properties map[objectResult:[commit]]"),
expectedError: errors.New("missing keys for these results which are required in TaskResult's properties map[objectResult:[commit]]"),
expectedResults: []v1.TaskRunResult{
{
Name: "aResult",
Expand Down
Loading
Loading