TEP-0090: Split up ResolvePipelineRunTask

[jerop]

Changes

Prior to this commit, the logic for resolving TaskRun and Resources needed for a given PipelineTask was included within the ResolvePipelineRunTask function.

In this change, we split the logic into their own functions for reuse and readability. We also fix the names of PipelineTask
variables to distinguish them from Tasks.

No functional changes made in this commit.

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

• [n/a] Docs included if any changes are user facing
• Tests included if any functionality added or changed
• Follows the commit message standard
• Meets the Tekton contributor standards (including functionality, content, code)
• Release notes block below has been filled in (if there are no user facing changes, use release note "NONE")

Release Notes

NONE

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	91.7%	-0.3

[jerop]

/test pull-tekton-pipeline-integration-tests

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	91.7%	-0.3

[jerop]

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-integration-tests

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.0%	-0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.2%	0.2

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.3%	0.2

[jerop]

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-integration-tests

there's a failure in TestTaskRunRetry that @abayer and I can't reproduce locally - and this change is unrelated to that test - https://tekton-releases.appspot.com/builds/tekton-prow/pr-logs/pull/tektoncd_pipeline/4943/pull-tekton-pipeline-integration-tests/

is it a flake? but it's happened several times 😕

update: fixed in #4944

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.2%	0.2

[abayer]

/assign

[abayer]

/retest

[abayer]

/lgtm

[abayer]

/lgtm

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.2%	0.2

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.2%	0.2

[abayer]

/retest

[pritidesai]

/lgtm

thank you @jerop 🎉

/test pull-tekton-pipeline-go-coverage

I am sorry to bring this up again but please add unit tests while introducing any new function 🙏

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.2%	0.2

[jerop]

I am sorry to bring this up again but please add unit tests while introducing any new function 🙏

@pritidesai we have guidelines - https://github.com/tektoncd/community/blob/main/standards.md#tests - to write tests for exported functions only...there was no exported function added, all new functions are unexported functions that are used in the tested exported function (plus there is no functionality change)...or do we need to update that guideline?

[pritidesai]

https://github.com/tektoncd/community/blob/main/standards.md#tests

I think we should, what do you think?

We (not intended to point on anyone) are adding a bunch of unexported functions which is great but its becoming very hard to read and understand the need of so many unexported functions. How are we sure that any changes we introduce are not breaking any functionality. This can only be made sure if we have 100% code coverage which is highly unlikely to maintain in any project.

Also, our standards today says the code coverage must remain same (at least). But thats not something enforced by our CI. One such example - #4927 (comment) - Impacting code coverage by 5% while refactoring will burn us down in the future.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	93.6%	0.2

[jerop]

https://github.com/tektoncd/community/blob/main/standards.md#tests

I think we should, what do you think?

We (not intended to point on anyone) are adding a bunch of unexported functions which is great but its becoming very hard to read and understand the need of so many unexported functions. How are we sure that any changes we introduce are not breaking any functionality. This can only be made sure if we have 100% code coverage which is highly unlikely to maintain in any project.

that's one option, we could also try out the suggestion in the guidelines to move those functions into packages: "If you find yourself wanting to test an unexported function, consider whether it would make sense to move the test into another package and export it"

added this topic to the agenda for the next Pipelines working group meeting on 06/14 (notes) so we can discuss with other maintainers

Also, our standards today says the code coverage must remain same (at least). But thats not something enforced by our CI. One such example - #4927 (comment) - Impacting code coverage by 5% while refactoring will burn us down in the future.

it'd be great if the CI enforced the code coverage standard, maybe for now we can add it to the submitter checklist to make it more explicit? we can bring it up in the working group meeting

[jerop]

@abayer @pritidesai - rebased the PR to fix merge conflicts after #4942 got merged - please take a look

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	93.3%	-0.1

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	93.4%	93.6%	0.2

[abayer]

/lgtm

[pritidesai]

that's one option, we could also try out the suggestion in the guidelines to move those functions into packages: "If you find yourself wanting to test an unexported function, consider whether it would make sense to move the test into another package and export it"

This suggestion can not be generalized, specially for the many complex packages we have in Pipelines. Unfortunately, it just resulting in a smelly code.

runNextSchedulableTask, processRunTimeouts, updateTaskRunsStatusDirectly, etc were all introduced on a similar basis and have grown complex over time without any unit testing. But they do follow the best practices - no unit test for unexported functions.

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 644 in 566e2e3

func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.PipelineRun, pipelineRunFacts *resources.PipelineRunFacts, as artifacts.ArtifactStorageInterface) error {

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 616 in 566e2e3

func (c *Reconciler) processRunTimeouts(ctx context.Context, pr *v1beta1.PipelineRun, pipelineState resources.PipelineRunState) error {

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 718 in 566e2e3

func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1beta1.PipelineRun) error {

I am not pointing at you @jerop, thanks for your understanding ❤️ Also, thank you for adding it to the WG for further discussion.

Didn't expect this to be merged as is, we have many functions with similar (complex) signatures now. At the same time, excited to see this result in much more meaningful and reusable code with matrix 🎉

pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

Lines 448 to 564 in 566e2e3

	func ResolvePipelineRunTask(
	ctx context.Context,
	pipelineRun v1beta1.PipelineRun,
	getTask resources.GetTask,
	getTaskRun resources.GetTaskRun,
	getRun GetRun,
	pipelineTask v1beta1.PipelineTask,
	providedResources map[string]*resourcev1alpha1.PipelineResource,
	) (*ResolvedPipelineRunTask, error) {
	rprt := ResolvedPipelineRunTask{
	PipelineTask: &pipelineTask,
	}
	rprt.CustomTask = isCustomTask(ctx, rprt)
	if rprt.IsCustomTask() {
	rprt.RunName = getRunName(pipelineRun.Status.Runs, pipelineRun.Status.ChildReferences, pipelineTask.Name, pipelineRun.Name)
	run, err := getRun(rprt.RunName)
	if err != nil && !kerrors.IsNotFound(err) {
	return nil, fmt.Errorf("error retrieving Run %s: %w", rprt.RunName, err)
	}
	rprt.Run = run
	} else {
	rprt.TaskRunName = GetTaskRunName(pipelineRun.Status.TaskRuns, pipelineRun.Status.ChildReferences, pipelineTask.Name, pipelineRun.Name)
	if err := rprt.resolvePipelineRunTaskWithTaskRun(ctx, rprt.TaskRunName, getTask, getTaskRun, pipelineTask, providedResources); err != nil {
	return nil, err
	}
	}
	return &rprt, nil
	}
	func (t *ResolvedPipelineRunTask) resolvePipelineRunTaskWithTaskRun(
	ctx context.Context,
	taskRunName string,
	getTask resources.GetTask,
	getTaskRun resources.GetTaskRun,
	pipelineTask v1beta1.PipelineTask,
	providedResources map[string]*resourcev1alpha1.PipelineResource,
	) error {
	taskRun, err := getTaskRun(taskRunName)
	if err != nil {
	if !kerrors.IsNotFound(err) {
	return fmt.Errorf("error retrieving TaskRun %s: %w", taskRunName, err)
	}
	}
	if taskRun != nil {
	t.TaskRun = taskRun
	}
	if err := t.resolveTaskResources(ctx, getTask, pipelineTask, providedResources, t.TaskRun); err != nil {
	return err
	}
	return nil
	}
	func (t *ResolvedPipelineRunTask) resolveTaskResources(
	ctx context.Context,
	getTask resources.GetTask,
	pipelineTask v1beta1.PipelineTask,
	providedResources map[string]*resourcev1alpha1.PipelineResource,
	taskRun *v1beta1.TaskRun,
	) error {
	spec, taskName, kind, err := resolveTask(ctx, taskRun, getTask, pipelineTask)
	if err != nil {
	return err
	}
	spec.SetDefaults(ctx)
	rtr, err := resolvePipelineTaskResources(pipelineTask, &spec, taskName, kind, providedResources)
	if err != nil {
	return fmt.Errorf("couldn't match referenced resources with declared resources: %w", err)
	}
	t.ResolvedTaskResources = rtr
	return nil
	}
	func resolveTask(
	ctx context.Context,
	taskRun *v1beta1.TaskRun,
	getTask resources.GetTask,
	pipelineTask v1beta1.PipelineTask,
	) (v1beta1.TaskSpec, string, v1beta1.TaskKind, error) {
	var (
	t v1beta1.TaskObject
	err error
	spec v1beta1.TaskSpec
	taskName string
	kind v1beta1.TaskKind
	)
	if pipelineTask.TaskRef != nil {
	// If the TaskRun has already a stored TaskSpec in its status, use it as source of truth
	if taskRun != nil && taskRun.Status.TaskSpec != nil {
	spec = *taskRun.Status.TaskSpec
	taskName = pipelineTask.TaskRef.Name
	} else {
	t, err = getTask(ctx, pipelineTask.TaskRef.Name)
	switch {
	case errors.Is(err, remote.ErrorRequestInProgress):
	return v1beta1.TaskSpec{}, "", "", err
	case err != nil:
	return v1beta1.TaskSpec{}, "", "", &TaskNotFoundError{
	Name: pipelineTask.TaskRef.Name,
	Msg: err.Error(),
	}
	default:
	spec = t.TaskSpec()
	taskName = t.TaskMetadata().Name
	}
	}
	kind = pipelineTask.TaskRef.Kind
	} else {
	spec = pipelineTask.TaskSpec.TaskSpec
	}
	return spec, taskName, kind, err
	}