Refactor ResolvedPipelineTask and remove redundant fields #6649

EmmaMunley · 2023-05-11T16:57:25Z

Changes

Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames, ObjectRuns, ObjectRunNames can be used regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue #6628.

/kind cleanup

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

None

tekton-robot · 2023-05-11T17:52:11Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.6%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0

tekton-robot · 2023-05-11T17:53:42Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.6%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0

tekton-robot · 2023-05-11T18:25:34Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.6%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0

tekton-robot · 2023-05-11T18:27:53Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	96.6%	-0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0

EmmaMunley · 2023-05-11T21:17:35Z

/assign @lbernick

pkg/reconciler/pipelinerun/resources/resultrefresolution.go

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

EmmaMunley

Thanks for the review @lbernick. Updated with comments below.

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

pkg/reconciler/pipelinerun/resources/resultrefresolution.go

EmmaMunley · 2023-05-12T16:44:33Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

@@ -112,9 +112,9 @@ func (state PipelineRunState) ToMap() map[string]*ResolvedPipelineTask {
 // IsBeforeFirstTaskRun returns true if the PipelineRun has not yet started its first TaskRun
 func (state PipelineRunState) IsBeforeFirstTaskRun() bool {
 for _, t := range state {
- if t.IsCustomTask() && t.RunObject != nil {
+ if t.IsCustomTask() && len(t.RunObjects) > 0 {


Updated and added test coverage for matrixed runs.

tekton-robot · 2023-05-12T16:54:25Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	94.4%	-2.3
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	98.9%	-0.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	91.2%	-0.7

tekton-robot · 2023-05-12T16:56:58Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	94.4%	-2.3
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	98.9%	-0.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	91.2%	-0.7

EmmaMunley · 2023-05-12T18:29:38Z

/retest
This appears to be a flake. Issue filed here: #6655

tekton-robot · 2023-05-12T18:29:39Z

@EmmaMunley: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-beta-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/retest This appears to be a flake. Issue filed here: #6655

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

EmmaMunley · 2023-05-12T18:30:30Z

/retest

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

lbernick

@EmmaMunley thanks for the updates! would you mind pulling the bug fixes (e.g. isBeforeFirstTaskRun) into a separate PR?

tekton-robot · 2023-05-15T14:58:41Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	94.4%	-2.3
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	98.9%	-0.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	91.2%	-0.7

tekton-robot · 2023-05-15T15:01:35Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.7%	94.4%	-2.3
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	98.9%	-0.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	91.2%	-0.7

EmmaMunley · 2023-05-18T14:43:29Z

/retest
#6680

pkg/reconciler/pipelinerun/pipelinerun.go

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

tekton-robot · 2023-05-18T16:08:44Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.3%	0.7
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

pkg/reconciler/pipelinerun/pipelinerun.go

tekton-robot · 2023-05-18T16:56:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.3%	0.7
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

tekton-robot · 2023-05-18T17:18:20Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.0%	0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

tekton-robot · 2023-05-18T17:23:07Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.0%	0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

tekton-robot · 2023-05-19T12:43:20Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.0%	0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

tekton-robot · 2023-05-19T12:45:52Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.0%	0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

Yongxuanzhang

In general lgtm, thanks for improving the code! There are lots of nice improvements

pkg/reconciler/pipelinerun/pipelinerun.go

Currently, PipelineRun resolution code can be difficult to understand. This commit removes some of the redundant fields in ResolvedPipelineTask including the singular version of: TaskRun, TaskRunName, ObjectRun, ObjectRunName so that only the list of TaskRuns, TaskRunNames, ObjectRuns, ObjectRunNames can be used regardless if the PipelineTask is matrixed or not. This also streamlines the codebase by removing the switch statements used throughout. This partially addresses Issue tektoncd#6628.

tekton-robot · 2023-05-22T15:24:38Z

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/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.0%	0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

tekton-robot · 2023-05-22T15:28:03Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	90.3%	1.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	96.6%	97.0%	0.4
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	-0.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.8%	92.3%	0.5

jerop

Thanks @EmmaMunley!

/lgtm

tekton-robot · 2023-05-22T17:25:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, lbernick, Yongxuanzhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jerop,lbernick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot requested review from imjasonh and pritidesai May 11, 2023 16:57

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 11, 2023

jerop self-assigned this May 11, 2023

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from c85b77b to 8a5774c Compare May 11, 2023 17:46

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2023

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from 8a5774c to 1127c9e Compare May 11, 2023 18:20

tekton-robot assigned lbernick May 11, 2023

lbernick reviewed May 12, 2023

View reviewed changes

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from 1127c9e to 6f03a8e Compare May 12, 2023 16:48

EmmaMunley commented May 12, 2023

View reviewed changes

EmmaMunley mentioned this pull request May 12, 2023

Flake: TestExamples workspace-volume #6655

Closed

lbernick reviewed May 15, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go Show resolved Hide resolved

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go Outdated Show resolved Hide resolved

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go Outdated Show resolved Hide resolved

lbernick reviewed May 15, 2023

View reviewed changes

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from 6f03a8e to 3fba1b8 Compare May 15, 2023 14:52

jerop reviewed May 18, 2023

View reviewed changes

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved

jerop reviewed May 18, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go Outdated Show resolved Hide resolved

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from 350e429 to 38c2e08 Compare May 18, 2023 16:02

Yongxuanzhang reviewed May 18, 2023

View reviewed changes

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved

jerop reviewed May 18, 2023

View reviewed changes

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from 38c2e08 to a791bad Compare May 18, 2023 17:12

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2023

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from a791bad to 156dc98 Compare May 19, 2023 12:37

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2023

EmmaMunley requested a review from jerop May 19, 2023 15:06

Yongxuanzhang approved these changes May 19, 2023

View reviewed changes

jerop reviewed May 22, 2023

View reviewed changes

pkg/reconciler/pipelinerun/pipelinerun.go Outdated Show resolved Hide resolved

EmmaMunley force-pushed the refactor-resolved-pipeline-task branch from 156dc98 to 88ed934 Compare May 22, 2023 15:19

jerop approved these changes May 22, 2023

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2023

tekton-robot merged commit 19af21c into tektoncd:main May 22, 2023

EmmaMunley deleted the refactor-resolved-pipeline-task branch May 22, 2023 17:28

lbernick mentioned this pull request May 22, 2023

Clean up pipelinerun resolution code #6628

Closed

EmmaMunley mentioned this pull request May 23, 2023

Consume Array Results in Matrix [Implementation] #6702

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ResolvedPipelineTask and remove redundant fields #6649

Refactor ResolvedPipelineTask and remove redundant fields #6649

EmmaMunley commented May 11, 2023 •

edited

Loading

tekton-robot commented May 11, 2023

tekton-robot commented May 11, 2023

tekton-robot commented May 11, 2023

tekton-robot commented May 11, 2023

EmmaMunley commented May 11, 2023

EmmaMunley left a comment •

edited

Loading

EmmaMunley May 12, 2023

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

EmmaMunley commented May 12, 2023 •

edited

Loading

tekton-robot commented May 12, 2023

EmmaMunley commented May 12, 2023

lbernick left a comment

tekton-robot commented May 15, 2023

tekton-robot commented May 15, 2023

EmmaMunley commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 19, 2023

tekton-robot commented May 19, 2023

Yongxuanzhang left a comment

tekton-robot commented May 22, 2023

tekton-robot commented May 22, 2023

jerop left a comment

tekton-robot commented May 22, 2023

Refactor ResolvedPipelineTask and remove redundant fields #6649

Refactor ResolvedPipelineTask and remove redundant fields #6649

Conversation

EmmaMunley commented May 11, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 11, 2023

tekton-robot commented May 11, 2023

tekton-robot commented May 11, 2023

tekton-robot commented May 11, 2023

EmmaMunley commented May 11, 2023

EmmaMunley left a comment • edited Loading

Choose a reason for hiding this comment

EmmaMunley May 12, 2023

Choose a reason for hiding this comment

tekton-robot commented May 12, 2023

tekton-robot commented May 12, 2023

EmmaMunley commented May 12, 2023 • edited Loading

tekton-robot commented May 12, 2023

EmmaMunley commented May 12, 2023

lbernick left a comment

Choose a reason for hiding this comment

tekton-robot commented May 15, 2023

tekton-robot commented May 15, 2023

EmmaMunley commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 19, 2023

tekton-robot commented May 19, 2023

Yongxuanzhang left a comment

Choose a reason for hiding this comment

tekton-robot commented May 22, 2023

tekton-robot commented May 22, 2023

jerop left a comment

Choose a reason for hiding this comment

tekton-robot commented May 22, 2023

EmmaMunley commented May 11, 2023 •

edited

Loading

EmmaMunley left a comment •

edited

Loading

EmmaMunley commented May 12, 2023 •

edited

Loading