From 7026ad6ef31eb5850f6440cfecd72c2d43afb5bc Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Fri, 24 Nov 2023 22:26:07 +0000 Subject: [PATCH 1/2] Cleanup outdated v1beta1 reference in pipelinerun reconciler --- pkg/reconciler/pipelinerun/resources/apply.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 5ed091dd11f..1e679d08d87 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -433,7 +433,7 @@ func ApplyTaskResultsToPipelineResults( } variableParts := strings.Split(variable, ".") - if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart { + if (variableParts[0] != v1.ResultTaskPart && variableParts[0] != v1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart { validPipelineResult = false invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name) continue @@ -470,7 +470,7 @@ func ApplyTaskResultsToPipelineResults( } else { // if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok { - if status != v1beta1.TaskRunReasonSuccessful.String() { + if status != v1.TaskRunReasonSuccessful.String() { validPipelineResult = false continue } @@ -494,7 +494,7 @@ func ApplyTaskResultsToPipelineResults( } else { // if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok { - if status != v1beta1.TaskRunReasonSuccessful.String() { + if status != v1.TaskRunReasonSuccessful.String() { validPipelineResult = false continue } From 5df6250e568ebda7fecd04429483a78797d9b616 Mon Sep 17 00:00:00 2001 From: JeromeJu Date: Tue, 5 Dec 2023 21:20:10 +0000 Subject: [PATCH 2/2] Add CouldntGetPipelineResult PipelineRunReason This commit adds the PipelineRunReasonCouldntGetPipelineResult which was previously conflated in PipelineRunReasonFailedValidation. It is now separated in order to keep the PipelineRunStatus failure reason granular. /kind cleanup --- docs/pipeline-api.md | 5 ++++ pkg/apis/pipeline/v1/pipelinerun_types.go | 4 +++ pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- pkg/reconciler/pipelinerun/resources/apply.go | 2 +- .../pipelinerun/resources/apply_test.go | 28 +++++++++---------- 5 files changed, 25 insertions(+), 16 deletions(-) diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 5705f9ced47..f07c32b8411 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -1968,6 +1968,11 @@ all of the running TaskRuns as cancelled failed.

ReasonCouldntGetPipeline indicates that the reason for the failure status is that the associated Pipeline couldn’t be retrieved

+

"CouldntGetPipelineResult"

+

PipelineRunReasonCouldntGetPipelineResult indicates that the pipeline fails to retrieve the +referenced result. This could be due to failed TaskRuns or Runs that were supposed to produce +the results

+

"CouldntGetTask"

ReasonCouldntGetTask indicates that the reason for the failure status is that the associated Pipeline’s Tasks couldn’t all be retrieved

diff --git a/pkg/apis/pipeline/v1/pipelinerun_types.go b/pkg/apis/pipeline/v1/pipelinerun_types.go index 26afd74faaf..b13aaa9f3a1 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types.go @@ -381,6 +381,10 @@ const ( // ReasonFailedValidation indicates that the reason for failure status is // that pipelinerun failed runtime validation PipelineRunReasonFailedValidation PipelineRunReason = "PipelineValidationFailed" + // PipelineRunReasonCouldntGetPipelineResult indicates that the pipeline fails to retrieve the + // referenced result. This could be due to failed TaskRuns or Runs that were supposed to produce + // the results + PipelineRunReasonCouldntGetPipelineResult PipelineRunReason = "CouldntGetPipelineResult" // ReasonInvalidGraph indicates that the reason for the failure status is that the // associated Pipeline is an invalid graph (a.k.a wrong order, cycle, …) PipelineRunReasonInvalidGraph PipelineRunReason = "PipelineInvalidGraph" diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 1f8d60d4c97..4ca005d451d 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -756,7 +756,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel pr.Status.Results, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results, pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pipelineRunFacts.GetPipelineTaskStatus()) if err != nil { - pr.Status.MarkFailed(v1.PipelineRunReasonFailedValidation.String(), err.Error()) + pr.Status.MarkFailed(v1.PipelineRunReasonCouldntGetPipelineResult.String(), err.Error()) return err } } diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 1e679d08d87..b76361b19c9 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -519,7 +519,7 @@ func ApplyTaskResultsToPipelineResults( } if len(invalidPipelineResults) > 0 { - return runResults, fmt.Errorf("invalid pipelineresults %v, the referred results don't exist", invalidPipelineResults) + return runResults, fmt.Errorf("invalid pipelineresults %v, the referenced results don't exist", invalidPipelineResults) } return runResults, nil diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 2e9b51f6008..29e75078dac 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -4083,7 +4083,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referenced results don't exist"), }, { description: "object-reference-key-not-exist", results: []v1.PipelineResult{{ @@ -4102,7 +4102,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referenced results don't exist"), }, { description: "object-results-resultname-not-exist", results: []v1.PipelineResult{{ @@ -4121,7 +4121,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referenced results don't exist"), }, { description: "invalid-result-variable-no-returned-result", results: []v1.PipelineResult{{ @@ -4135,7 +4135,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "no-taskrun-results-no-returned-results", results: []v1.PipelineResult{{ @@ -4146,7 +4146,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { "pt1": {}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "invalid-taskrun-name-no-returned-result", results: []v1.PipelineResult{{ @@ -4160,7 +4160,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "invalid-result-name-no-returned-result", results: []v1.PipelineResult{{ @@ -4174,7 +4174,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "unsuccessful-taskrun-no-returned-result", results: []v1.PipelineResult{{ @@ -4183,7 +4183,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }}, taskResults: map[string][]v1.TaskRunResult{}, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "mixed-success-tasks-some-returned-results", results: []v1.PipelineResult{{ @@ -4203,7 +4203,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { Name: "bar", Value: *v1.NewStructuredValues("rae"), }}, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "no-run-results-no-returned-results", results: []v1.PipelineResult{{ @@ -4212,7 +4212,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }}, runResults: map[string][]v1beta1.CustomRunResult{}, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "wrong-customtask-name-no-returned-result", results: []v1.PipelineResult{{ @@ -4226,7 +4226,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "right-customtask-name-wrong-result-name-no-returned-result", results: []v1.PipelineResult{{ @@ -4240,7 +4240,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { }}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "unsuccessful-run-no-returned-result", results: []v1.PipelineResult{{ @@ -4251,7 +4251,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { "customtask": {}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }, { description: "wrong-result-reference-expression", results: []v1.PipelineResult{{ @@ -4262,7 +4262,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { "customtask": {}, }, expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referenced results don't exist"), }} { t.Run(tc.description, func(t *testing.T) { received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*skipped tasks*/)