From 6789d941cff1ec1f694cef4ff4280edfd99c38b0 Mon Sep 17 00:00:00 2001 From: divyansh42 Date: Wed, 16 Oct 2024 15:47:18 +0530 Subject: [PATCH] Resolve review comments 1 Signed-off-by: divyansh42 --- pkg/reconciler/pipelinerun/pipelinerun.go | 32 +++++++------------ .../resources/pipelinerunresolution.go | 14 ++++---- .../resources/resultrefresolution_test.go | 2 +- 3 files changed, 20 insertions(+), 28 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index e6de2d0d78e..4600d8cfe65 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -833,33 +833,25 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1.Pipeline recorder := controller.GetEventRecorder(ctx) // nextRpts holds a list of pipeline tasks which should be executed next - // tmpNextRpts holds the nextRpts temporarily, - // tmpNextRpts is later filtered to check for the missing result reference - // if the pipelineTask is valid then it is added to the nextRpts - tmpNextRpts, err := pipelineRunFacts.DAGExecutionQueue() + nextRpts, err := pipelineRunFacts.DAGExecutionQueue() if err != nil { logger.Errorf("Error getting potential next tasks for valid pipelinerun %s: %v", pr.Name, err) return controller.NewPermanentError(err) } - var nextRpts resources.PipelineRunState - for _, nextRpt := range tmpNextRpts { - // Check for Missing Result References and - // store the faulty task in missingRefTask - missingRefTask, err := resources.CheckMissingResultReferences(pipelineRunFacts.State, nextRpt) + for _, rpt := range nextRpts { + // Check for Missing Result References + // if error found, present rpt will be + // added to the validationFailedTask list + err := resources.CheckMissingResultReferences(pipelineRunFacts.State, rpt) if err != nil { logger.Infof("Failed to resolve task result reference for %q with error %v", pr.Name, err) - pr.Status.MarkFailed(v1.PipelineRunReasonInvalidTaskResultReference.String(), err.Error()) - // check if pipeline contains finally tasks - // return the permanent error only if there is no finally task - fTaskNames := pipelineRunFacts.GetFinalTaskNames() - pipelineRunFacts.ValidationFailedTask = append(pipelineRunFacts.ValidationFailedTask, missingRefTask) - if len(fTaskNames) == 0 { - return controller.NewPermanentError(err) - } - } else { - // if task is valid then add it to nextRpts for the further execution - nextRpts = append(nextRpts, nextRpt) + // If there is an error encountered, no new task + // will be scheduled, hence nextRpts should be empty + // if finally tasks are found, then those tasks will + // be added to the nextRpts + nextRpts = nil + pipelineRunFacts.ValidationFailedTask = append(pipelineRunFacts.ValidationFailedTask, rpt) } } // GetFinalTasks only returns final tasks when a DAG is complete diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index d6f3d8046ba..211eb7bd458 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -835,33 +835,33 @@ func isCustomRunCancelledByPipelineRunTimeout(cr *v1beta1.CustomRun) bool { // CheckMissingResultReferences returns an error if it is missing any result references. // Missing result references can occur if task fails to produce a result but has // OnError: continue (ie TestMissingResultWhenStepErrorIsIgnored) -func CheckMissingResultReferences(pipelineRunState PipelineRunState, target *ResolvedPipelineTask) (*ResolvedPipelineTask, error) { +func CheckMissingResultReferences(pipelineRunState PipelineRunState, target *ResolvedPipelineTask) error { for _, resultRef := range v1.PipelineTaskResultRefs(target.PipelineTask) { referencedPipelineTask, ok := pipelineRunState.ToMap()[resultRef.PipelineTask] if !ok { - return target, fmt.Errorf("Result reference error: Could not find ref \"%s\" in internal pipelineRunState", resultRef.PipelineTask) + return fmt.Errorf("Result reference error: Could not find ref \"%s\" in internal pipelineRunState", resultRef.PipelineTask) } if referencedPipelineTask.IsCustomTask() { if len(referencedPipelineTask.CustomRuns) == 0 { - return target, fmt.Errorf("Result reference error: Internal result ref \"%s\" has zero-length CustomRuns", resultRef.PipelineTask) + return fmt.Errorf("Result reference error: Internal result ref \"%s\" has zero-length CustomRuns", resultRef.PipelineTask) } customRun := referencedPipelineTask.CustomRuns[0] _, err := findRunResultForParam(customRun, resultRef) if err != nil { - return target, err + return err } } else { if len(referencedPipelineTask.TaskRuns) == 0 { - return target, fmt.Errorf("Result reference error: Internal result ref \"%s\" has zero-length TaskRuns", resultRef.PipelineTask) + return fmt.Errorf("Result reference error: Internal result ref \"%s\" has zero-length TaskRuns", resultRef.PipelineTask) } taskRun := referencedPipelineTask.TaskRuns[0] _, err := findTaskResultForParam(taskRun, resultRef) if err != nil { - return target, err + return err } } } - return target, nil + return nil } // createResultsCacheMatrixedTaskRuns creates a cache of results that have been fanned out from a diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go index 0d7ce87d1c7..1658445abcf 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go @@ -817,7 +817,7 @@ func TestCheckMissingResultReferences(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var err error for _, target := range tt.targets { - _, tmpErr := CheckMissingResultReferences(tt.pipelineRunState, target) + tmpErr := CheckMissingResultReferences(tt.pipelineRunState, target) if tmpErr != nil { err = tmpErr }