refactor: improve names and fix typos

While working on TEP-0056 small sets of typo fixes and minor
refactorings are extracted as small separated PRs to reduce the
cognitive load for the actual TEP-0056 code changes in following PRs.

Issues #8760, #7166.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>

Author: twoGiants

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -332,29 +332,49 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, pr *v1
 	return errs
 }
 
-// resolvePipelineState will attempt to resolve each referenced task in the pipeline's spec and all of the resources
+// resolvePipelineState will attempt to resolve each referenced pipeline task in the pipeline's spec and all of the resources
 // specified by those tasks.
 func (c *Reconciler) resolvePipelineState(
 	ctx context.Context,
-	tasks []v1.PipelineTask,
+	pipelineTasks []v1.PipelineTask,
 	pipelineMeta *metav1.ObjectMeta,
 	pr *v1.PipelineRun,
 	pst resources.PipelineRunState,
 ) (resources.PipelineRunState, error) {
 	ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "resolvePipelineState")
 	defer span.End()
-	// Resolve each task individually because they each could have a different reference context (remote or local).
-	for _, task := range tasks {
+	// Resolve each pipeline task individually because they each could have a different reference context (remote or local).
+	for _, pipelineTask := range pipelineTasks {
 		// We need the TaskRun name to ensure that we don't perform an additional remote resolution request for a PipelineTask
 		// in the TaskRun reconciler.
-		trName := resources.GetTaskRunName(pr.Status.ChildReferences, task.Name, pr.Name)
+		trName := resources.GetTaskRunName(
+			pr.Status.ChildReferences,
+			pipelineTask.Name,
+			pr.Name,
+		)
 
 		// list VerificationPolicies for trusted resources
 		vp, err := c.verificationPolicyLister.VerificationPolicies(pr.Namespace).List(labels.Everything())
 		if err != nil {
 			return nil, fmt.Errorf("failed to list VerificationPolicies from namespace %s with error %w", pr.Namespace, err)
 		}
-		fn := tresources.GetTaskFunc(ctx, c.KubeClientSet, c.PipelineClientSet, c.resolutionRequester, pr, task.TaskRef, trName, pr.Namespace, pr.Spec.TaskRunTemplate.ServiceAccountName, vp)
+
+		getTaskFunc := tresources.GetTaskFunc(
+			ctx,
+			c.KubeClientSet,
+			c.PipelineClientSet,
+			c.resolutionRequester,
+			pr,
+			pipelineTask.TaskRef,
+			trName,
+			pr.Namespace,
+			pr.Spec.TaskRunTemplate.ServiceAccountName,
+			vp,
+		)
+
+		getTaskRunFunc := func(name string) (*v1.TaskRun, error) {
+			return c.taskRunLister.TaskRuns(pr.Namespace).Get(name)
+		}
 
 		getCustomRunFunc := func(name string) (*v1beta1.CustomRun, error) {
 			r, err := c.customRunLister.CustomRuns(pr.Namespace).Get(name)
@@ -366,12 +386,10 @@ func (c *Reconciler) resolvePipelineState(
 
 		resolvedTask, err := resources.ResolvePipelineTask(ctx,
 			*pr,
-			fn,
-			func(name string) (*v1.TaskRun, error) {
-				return c.taskRunLister.TaskRuns(pr.Namespace).Get(name)
-			},
+			getTaskFunc,
+			getTaskRunFunc,
 			getCustomRunFunc,
-			task,
+			pipelineTask,
 			pst,
 		)
 		if err != nil {
@@ -393,8 +411,9 @@ func (c *Reconciler) resolvePipelineState(
 			}
 			return nil, controller.NewPermanentError(err)
 		}
+
 		if resolvedTask.ResolvedTask != nil && resolvedTask.ResolvedTask.VerificationResult != nil {
-			cond, err := conditionFromVerificationResult(resolvedTask.ResolvedTask.VerificationResult, pr, task.Name)
+			cond, err := conditionFromVerificationResult(resolvedTask.ResolvedTask.VerificationResult, pr, pipelineTask.Name)
 			pr.Status.SetCondition(cond)
 			if err != nil {
 				pr.Status.MarkFailed(v1.PipelineRunReasonResourceVerificationFailed.String(), err.Error())
@@ -403,6 +422,7 @@ func (c *Reconciler) resolvePipelineState(
 		}
 		pst = append(pst, resolvedTask)
 	}
+
 	return pst, nil
 }
 
@@ -549,7 +569,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 	}
 
 	resources.ApplyParametersToWorkspaceBindings(ctx, pr)
-	// Make a deep copy of the Pipeline and its Tasks before value substution.
+	// Make a deep copy of the Pipeline and its Tasks before value substitution.
 	// This is used to find referenced pipeline-level params at each PipelineTask when validate param enum subset requirement
 	originalPipeline := pipelineSpec.DeepCopy()
 	originalTasks := originalPipeline.Tasks
@@ -571,9 +591,9 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 		return controller.NewPermanentError(err)
 	}
 
-	// pipelineState holds a list of pipeline tasks after fetching their resolved Task specs.
-	// pipelineState also holds a taskRun for each pipeline task after the taskRun is created
-	// pipelineState is instantiated and updated on every reconcile cycle
+	// pipelineRunState holds a list of pipeline tasks after fetching their resolved Task specs.
+	// pipelineRunState also holds a taskRun for each pipeline task after the taskRun is created
+	// pipelineRunState is instantiated and updated on every reconcile cycle
 	// Resolve the set of tasks (and possibly task runs).
 	tasks := pipelineSpec.Tasks
 	if len(pipelineSpec.Finally) > 0 {
@@ -584,7 +604,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 	// - those with a completed (Task|Custom)Run reference (i.e. those that finished running)
 	// - those without a (Task|Custom)Run reference
 	// We resolve the status for the former first, to collect all results available at this stage
-	// We know that tasks in progress or completed have had their fan-out alteady calculated so
+	// We know that tasks in progress or completed have had their fan-out already calculated so
 	// they can be safely processed in the first iteration. The underlying assumption is that if
 	// a PipelineTask has at least one TaskRun associated, then all its TaskRuns have been
 	// created already.
@@ -604,9 +624,9 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 			notStartedTasks = append(notStartedTasks, task)
 		}
 	}
+
 	// First iteration
-	pst := resources.PipelineRunState{}
-	pipelineRunState, err := c.resolvePipelineState(ctx, ranOrRunningTasks, pipelineMeta.ObjectMeta, pr, pst)
+	pipelineRunState, err := c.resolvePipelineState(ctx, ranOrRunningTasks, pipelineMeta.ObjectMeta, pr, resources.PipelineRunState{})
 	switch {
 	case errors.Is(err, remote.ErrRequestInProgress):
 		message := fmt.Sprintf("PipelineRun %s/%s awaiting remote resource", pr.Namespace, pr.Name)
@@ -692,7 +712,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 		}
 	}
 
-	// check if pipeline run is not gracefully cancelled and there are active task runs, which require cancelling
+	// check if pipeline run is gracefully cancelled and there are active pipeline task runs, which require cancelling
 	if pr.IsGracefullyCancelled() && pipelineRunFacts.IsRunning() {
 		// If the pipelinerun is cancelled, cancel tasks, but run finally
 		err := gracefullyCancelPipelineRun(ctx, logger, pr, c.PipelineClientSet)
@@ -781,6 +801,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 			}
 		}
 	}
+
 	if err := c.runNextSchedulableTask(ctx, pr, pipelineRunFacts); err != nil {
 		return err
 	}
@@ -811,14 +832,18 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 	pr.Status.ChildReferences = pipelineRunFacts.GetChildReferences()
 
 	pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
-
-	taskStatus := pipelineRunFacts.GetPipelineTaskStatus()
-	finalTaskStatus := pipelineRunFacts.GetPipelineFinalTaskStatus()
-	taskStatus = kmap.Union(taskStatus, finalTaskStatus)
+	pipelineTaskStatus := pipelineRunFacts.GetPipelineTaskStatus()
+	finalPipelineTaskStatus := pipelineRunFacts.GetPipelineFinalTaskStatus()
+	pipelineTaskStatus = kmap.Union(pipelineTaskStatus, finalPipelineTaskStatus)
 
 	if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse {
-		pr.Status.Results, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results,
-			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), taskStatus)
+		pr.Status.Results, err = resources.ApplyTaskResultsToPipelineResults(
+			ctx,
+			pipelineSpec.Results,
+			pipelineRunFacts.State.GetTaskRunsResults(),
+			pipelineRunFacts.State.GetRunsResults(),
+			pipelineTaskStatus,
+		)
 		if err != nil {
 			pr.Status.MarkFailed(v1.PipelineRunReasonCouldntGetPipelineResult.String(),
 				"Failed to get PipelineResult from TaskRun Results for PipelineRun %s: %s",
@@ -944,6 +969,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1.Pipeline
 			}
 		}
 	}
+
 	return nil
 }
 
@@ -960,11 +986,12 @@ func (c *Reconciler) setFinallyStartedTimeIfNeeded(pr *v1.PipelineRun, facts *re
 func (c *Reconciler) createTaskRuns(ctx context.Context, rpt *resources.ResolvedPipelineTask, pr *v1.PipelineRun, facts *resources.PipelineRunFacts) ([]*v1.TaskRun, error) {
 	ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "createTaskRuns")
 	defer span.End()
-	var taskRuns []*v1.TaskRun
+
 	var matrixCombinations []v1.Params
 	if rpt.PipelineTask.IsMatrixed() {
 		matrixCombinations = rpt.PipelineTask.Matrix.FanOut()
 	}
+
 	// validate the param values meet resolved Task Param Enum requirements before creating TaskRuns
 	if config.FromContextOrDefaults(ctx).FeatureFlags.EnableParamEnum {
 		for i := range rpt.TaskRunNames {
@@ -981,6 +1008,8 @@ func (c *Reconciler) createTaskRuns(ctx context.Context, rpt *resources.Resolved
 			}
 		}
 	}
+
+	var taskRuns []*v1.TaskRun
 	for i, taskRunName := range rpt.TaskRunNames {
 		var params v1.Params
 		if len(matrixCombinations) > i {
@@ -993,6 +1022,7 @@ func (c *Reconciler) createTaskRuns(ctx context.Context, rpt *resources.Resolved
 		}
 		taskRuns = append(taskRuns, taskRun)
 	}
+
 	return taskRuns, nil
 }
 

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -1360,7 +1360,7 @@ func TestReconcile_InvalidPipelineRunNames(t *testing.T) {
 func TestReconcileOnCompletedPipelineRun(t *testing.T) {
 	// TestReconcileOnCompletedPipelineRun runs "Reconcile" on a PipelineRun that already reached completion
 	// and that does not have the latest status from TaskRuns yet. It checks that the TaskRun status is updated
-	// in the PipelineRun status, that the completion status is not altered, that not error is returned and
+	// in the PipelineRun status, that the completion status is not altered, that no error is returned and
 	// a successful event is triggered
 	namespace := "foo"
 	taskRunName := "test-pipeline-run-completed-hello-world-task-run"

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -65,13 +65,16 @@ func (e *TaskNotFoundError) Error() string {
 type ResolvedPipelineTask struct {
 	TaskRunNames []string
 	TaskRuns     []*v1.TaskRun
+	ResolvedTask *resources.ResolvedTask
+
 	// If the PipelineTask is a Custom Task, CustomRunName and CustomRun will be set.
 	CustomTask     bool
 	CustomRunNames []string
 	CustomRuns     []*v1beta1.CustomRun
-	PipelineTask   *v1.PipelineTask
-	ResolvedTask   *resources.ResolvedTask
-	ResultsCache   map[string][]string
+
+	PipelineTask *v1.PipelineTask
+	ResultsCache map[string][]string
+
 	// EvaluatedCEL is used to store the results of evaluated CEL expression
 	EvaluatedCEL map[string]bool
 }
@@ -95,7 +98,7 @@ func (t *ResolvedPipelineTask) EvaluateCEL() error {
 				if iss.Err() != nil {
 					return iss.Err()
 				}
-				// Generate an evaluable instance of the Ast within the environment
+				// Generate an evaluatable instance of the Ast within the environment
 				prg, err := env.Program(ast)
 				if err != nil {
 					return err
@@ -156,6 +159,7 @@ func (t ResolvedPipelineTask) getReason() string {
 			return t.CustomRuns[0].Status.Conditions[0].Reason
 		}
 	}
+
 	if len(t.TaskRuns) == 0 {
 		return ""
 	}
@@ -167,6 +171,7 @@ func (t ResolvedPipelineTask) getReason() string {
 	if len(t.TaskRuns) >= 1 && len(t.TaskRuns[0].Status.Conditions) >= 1 {
 		return t.TaskRuns[0].Status.Conditions[0].Reason
 	}
+
 	return ""
 }
 
@@ -184,6 +189,7 @@ func (t ResolvedPipelineTask) isSuccessful() bool {
 		}
 		return true
 	}
+
 	if len(t.TaskRuns) == 0 {
 		return false
 	}
@@ -309,10 +315,11 @@ func (t ResolvedPipelineTask) haveAnyRunsFailed() bool {
 	if t.IsCustomTask() {
 		return t.haveAnyCustomRunsFailed()
 	}
+
 	return t.haveAnyTaskRunsFailed()
 }
 
-// haveAnyTaskRunsFailed returns true when any of the taskRuns have succeeded condition with status set to false
+// haveAnyTaskRunsFailed returns true when any of the TaskRuns have succeeded condition with status set to false
 func (t ResolvedPipelineTask) haveAnyTaskRunsFailed() bool {
 	for _, taskRun := range t.TaskRuns {
 		if taskRun.IsFailure() {
@@ -322,7 +329,7 @@ func (t ResolvedPipelineTask) haveAnyTaskRunsFailed() bool {
 	return false
 }
 
-// haveAnyCustomRunsFailed returns true when a CustomRun has succeeded condition with status set to false
+// haveAnyCustomRunsFailed returns true when any of the CustomRuns have succeeded condition with status set to false
 func (t ResolvedPipelineTask) haveAnyCustomRunsFailed() bool {
 	for _, customRun := range t.CustomRuns {
 		if customRun.IsFailure() {
@@ -633,6 +640,7 @@ func ResolvePipelineTask(
 			}
 		}
 	}
+
 	return &rpt, nil
 }
 
@@ -748,27 +756,27 @@ func getTaskRunNamesFromChildRefs(childRefs []v1.ChildStatusReference, ptName st
 }
 
 func getNewRunNames(ptName, prName string, numberOfRuns int) []string {
-	var taskRunNames []string
+	var runNames []string
 	// If it is a singular TaskRun/CustomRun, we only append the ptName
 	if numberOfRuns == 1 {
-		taskRunName := kmeta.ChildName(prName, "-"+ptName)
-		return append(taskRunNames, taskRunName)
+		runName := kmeta.ChildName(prName, "-"+ptName)
+		return append(runNames, runName)
 	}
-	// For a matrix we append i to then end of the fanned out TaskRuns "matrixed-pr-taskrun-0"
+	// For a matrix we append i to the end of the fanned out TaskRuns/CustomRun "matrixed-pr-taskrun-0"
 	for i := range numberOfRuns {
-		taskRunName := kmeta.ChildName(prName, fmt.Sprintf("-%s-%d", ptName, i))
+		runName := kmeta.ChildName(prName, fmt.Sprintf("-%s-%d", ptName, i))
 		// check if the taskRun name ends with a matrix instance count
-		if !strings.HasSuffix(taskRunName, fmt.Sprintf("-%d", i)) {
-			taskRunName = kmeta.ChildName(prName, "-"+ptName)
+		if !strings.HasSuffix(runName, fmt.Sprintf("-%d", i)) {
+			runName = kmeta.ChildName(prName, "-"+ptName)
 			// kmeta.ChildName limits the size of a name to max of 63 characters based on k8s guidelines
-			// truncate the name such that "-<matrix-id>" can be appended to the taskRun name
+			// truncate the name such that "-<matrix-id>" can be appended to the TaskRun/CustomRun name
 			longest := 63 - len(fmt.Sprintf("-%d", numberOfRuns))
-			taskRunName = taskRunName[0:longest]
-			taskRunName = fmt.Sprintf("%s-%d", taskRunName, i)
+			runName = runName[0:longest]
+			runName = fmt.Sprintf("%s-%d", runName, i)
 		}
-		taskRunNames = append(taskRunNames, taskRunName)
+		runNames = append(runNames, runName)
 	}
-	return taskRunNames
+	return runNames
 }
 
 // getCustomRunName should return a unique name for a `Run` if one has not already
@@ -866,7 +874,7 @@ func CheckMissingResultReferences(pipelineRunState PipelineRunState, target *Res
 }
 
 // createResultsCacheMatrixedTaskRuns creates a cache of results that have been fanned out from a
-// referenced matrixed PipelintTask so that you can easily access these results in subsequent Pipeline Tasks
+// referenced matrixed PipelineTask so that you can easily access these results in subsequent Pipeline Tasks
 func createResultsCacheMatrixedTaskRuns(rpt *ResolvedPipelineTask) (resultsCache map[string][]string) {
 	if len(rpt.ResultsCache) == 0 {
 		resultsCache = make(map[string][]string)