Switch ApplyTaskResultsToPipelineResults to not use status maps #4753

abayer · 2022-04-12T21:23:45Z

Changes

This comes out of discussions on #4739 - with the new minimal embedded status
changes which will be introduced in that PR, we can see that we're currently
using the output of pipelineRunFacts.State.GetTaskRunsStatus(pr) and
pipelineRunFacts.State.GetRunsStatus(pr) for two separate purposes:

To set pr.Status.TaskRuns and pr.Status.Runs with the full embedded status
To pass to resources.ApplyTaskResultsToPipelineResults for populating results

It's understandable why ApplyTaskResultsToPipelineResults is using
the maps from pr.Status.[TaskRuns|Runs], since those maps do contain everything
needed for propagating results up from the tasks to the pipeline run, but if you
look at the current implementation, you can see that it's shuffling the maps
around into a different form that's more suited for what it's doing than the
original form.

So this PR reworks ApplyTaskResultsToPipelineResults to instead take maps in
the form the current implementation uses internally, with new functions on
PipelineRunState to get these new maps without needing to use the pr.Status.[TaskRuns|Runs]
form as an intermediary. This makes the pre-minimal-embedded-status
implementation cleaner, and is particularly helpful in that regard once we do
have minimal embedded status in place.

/kind misc

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

abayer · 2022-04-12T21:23:57Z

/assign @lbernick
/assign @pritidesai

tekton-robot · 2022-04-12T21:26:10Z

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/pipelinerunstate.go	96.7%	96.6%	-0.1

vdemeester · 2022-04-13T05:52:53Z

pkg/reconciler/pipelinerun/resources/apply.go

@@ -21,10 +21,10 @@ import (
 	"strconv"
 	"strings"

+	"github.com/tektoncd/pipeline/pkg/apis/run/v1alpha1"
+


tekton-robot · 2022-04-13T05:53:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

lbernick · 2022-04-13T13:06:19Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

+		if rprt.TaskRun == nil {
+			continue
+		}
+		cond := rprt.TaskRun.Status.Status.GetCondition(apis.ConditionSucceeded)


you may be able to use this function (for taskRun) or this one (for RPRT)
although not sure how you want to do the nil check. Same comment for Runs below.

I would also update the last line of the docstring to say "it returns results only for successful TaskRuns" (and same for runs)

I think that second function is perfect - switching to it now.

lbernick · 2022-04-13T13:09:59Z

pkg/reconciler/pipelinerun/resources/apply.go

-	}
-
-	for _, trResult := range status.Status.TaskRunResults {
+func taskResultValue(taskName string, resultName string, taskResults map[string][]v1beta1.TaskRunResult) *string {


This docstring and the one for runResultValue should be updated.

tekton-robot · 2022-04-13T13:32:42Z

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/pipelinerunstate.go	96.7%	96.9%	0.2

lbernick · 2022-04-13T13:39:15Z

/lgtm

abayer · 2022-04-13T14:03:34Z

/retest

lbernick · 2022-04-13T15:13:28Z

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

This comes out of discussions on tektoncd#4739 - with the new minimal embedded status changes which will be introduced in that PR, we can see that we're currently using the output of `pipelineRunFacts.State.GetTaskRunsStatus(pr)` and `pipelineRunFacts.State.GetRunsStatus(pr)` for two separate purposes: * To set `pr.Status.TaskRuns` and `pr.Status.Runs` with the full embedded status * To pass to `resources.ApplyTaskResultsToPipelineResults` for populating results It's understandable why `ApplyTaskResultsToPipelineResults` is using the maps from `pr.Status.[TaskRuns|Runs]`, since those maps do contain everything needed for propagating results up from the tasks to the pipeline run, but if you look at the current implementation, you can see that it's shuffling the maps around into a different form that's more suited for what it's doing than the original form. So this PR reworks `ApplyTaskResultsToPipelineResults` to instead take maps in the form the current implementation uses internally, with new functions on `PipelineRunState` to get these new maps without needing to use the `pr.Status.[TaskRuns|Runs]` form as an intermediary. This makes the pre-minimal-embedded-status implementation cleaner, and is particularly helpful in that regard once we do have minimal embedded status in place. Signed-off-by: Andrew Bayer <[email protected]>

lbernick · 2022-04-13T16:40:35Z

/lgtm

tekton-robot · 2022-04-13T16:42:40Z

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/pipelinerunstate.go	96.7%	96.9%	0.2

…d status This builds on tektoncd#4694, tektoncd#4734, and tektoncd#4753. It will feed into a revamped tektoncd#4739, all as part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md and tektoncd#3140. Specifically, this adds functionality to `pkg/reconciler/pipelinerun/resources` in `pipelinerunresolution.go` and `pipelinerunstate.go` which will be needed for the full implementation. These changes won't have any effects in the current situation, because `pr.Status.ChildReferences` is never populated, so can be made independently of the rest of the implementation, thus also shrinking the size of the rest of the implementation PR(s) for easier review. Signed-off-by: Andrew Bayer <[email protected]>

…d status This builds on #4694, #4734, and #4753. It will feed into a revamped #4739, all as part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md and #3140. Specifically, this adds functionality to `pkg/reconciler/pipelinerun/resources` in `pipelinerunresolution.go` and `pipelinerunstate.go` which will be needed for the full implementation. These changes won't have any effects in the current situation, because `pr.Status.ChildReferences` is never populated, so can be made independently of the rest of the implementation, thus also shrinking the size of the rest of the implementation PR(s) for easier review. Signed-off-by: Andrew Bayer <[email protected]>

And of course, this is part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md, building on a pile of other PRs (tektoncd#4705, tektoncd#4734, tektoncd#4753, tektoncd#4757). This adds a new function to `pkg/reconciler/pipelinerun/pipelinerun.go`, specifically for updating `pr.Status.ChildReferences` during reconciliation. It's analogous to the existing `updatePipelineRunStatusFromTaskRuns` and `updatePipelineRunStatusFromRuns` functions. This PR doesn't actually call the new function - behavior is exactly the same. But it adds the new function, along with other functions it depends on. In the final step of the implementation, these other functions will also be used in `...FromTaskRuns` and/or `...FromRuns`. I also reworked `pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go` to improve its test fixtures, so that they're easier to reuse and instantiated via YAML parsing as much as possible. Signed-off-by: Andrew Bayer <[email protected]>

And of course, this is part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md, building on a pile of other PRs (#4705, #4734, #4753, #4757). This adds a new function to `pkg/reconciler/pipelinerun/pipelinerun.go`, specifically for updating `pr.Status.ChildReferences` during reconciliation. It's analogous to the existing `updatePipelineRunStatusFromTaskRuns` and `updatePipelineRunStatusFromRuns` functions. This PR doesn't actually call the new function - behavior is exactly the same. But it adds the new function, along with other functions it depends on. In the final step of the implementation, these other functions will also be used in `...FromTaskRuns` and/or `...FromRuns`. I also reworked `pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go` to improve its test fixtures, so that they're easier to reuse and instantiated via YAML parsing as much as possible. Signed-off-by: Andrew Bayer <[email protected]>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#4734 * tektoncd#4753 * tektoncd#4757 * tektoncd#4760 * tektoncd#3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of all the other PRs referenced above. Signed-off-by: Andrew Bayer <[email protected]>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * #4705 * #4734 * #4753 * #4757 * #4760 * #3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of all the other PRs referenced above. Signed-off-by: Andrew Bayer <[email protected]>

…d status This builds on tektoncd#4694, tektoncd#4734, and tektoncd#4753. It will feed into a revamped tektoncd#4739, all as part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md and tektoncd#3140. Specifically, this adds functionality to `pkg/reconciler/pipelinerun/resources` in `pipelinerunresolution.go` and `pipelinerunstate.go` which will be needed for the full implementation. These changes won't have any effects in the current situation, because `pr.Status.ChildReferences` is never populated, so can be made independently of the rest of the implementation, thus also shrinking the size of the rest of the implementation PR(s) for easier review. Signed-off-by: Andrew Bayer <[email protected]>

And of course, this is part of https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md, building on a pile of other PRs (tektoncd#4705, tektoncd#4734, tektoncd#4753, tektoncd#4757). This adds a new function to `pkg/reconciler/pipelinerun/pipelinerun.go`, specifically for updating `pr.Status.ChildReferences` during reconciliation. It's analogous to the existing `updatePipelineRunStatusFromTaskRuns` and `updatePipelineRunStatusFromRuns` functions. This PR doesn't actually call the new function - behavior is exactly the same. But it adds the new function, along with other functions it depends on. In the final step of the implementation, these other functions will also be used in `...FromTaskRuns` and/or `...FromRuns`. I also reworked `pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go` to improve its test fixtures, so that they're easier to reuse and instantiated via YAML parsing as much as possible. Signed-off-by: Andrew Bayer <[email protected]>

…pelineRuns See: * https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md * tektoncd#4705 * tektoncd#4734 * tektoncd#4753 * tektoncd#4757 * tektoncd#4760 * tektoncd#3140 This implements TEP-0100, allowing for choosing between the original full embedded `TaskRun` and `Run` statuses in `PipelineRun` statuses, minimal child references to the underlying `TaskRun` and `Run`s, or both, building on top of all the other PRs referenced above. Signed-off-by: Andrew Bayer <[email protected]>

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Apr 12, 2022

tekton-robot requested review from bobcatfish and vdemeester April 12, 2022 21:23

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 12, 2022

tekton-robot assigned lbernick and pritidesai Apr 12, 2022

vdemeester approved these changes Apr 13, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2022

lbernick reviewed Apr 13, 2022

View reviewed changes

abayer force-pushed the simplify-taskresults-to-pipelineresults branch from 0b56d80 to 4555ea6 Compare April 13, 2022 13:28

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022

abayer force-pushed the simplify-taskresults-to-pipelineresults branch from 4555ea6 to dfc2fed Compare April 13, 2022 16:39

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2022

tekton-robot merged commit cb23f45 into tektoncd:main Apr 13, 2022

abayer mentioned this pull request Apr 13, 2022

[TEP-0100] Add functionality to be used in supporting minimal embedded status #4757

Merged

5 tasks

abayer mentioned this pull request Apr 14, 2022

[TEP-0100] Add new updatePipelineRunStatusFromChildRefs function #4760

Merged

5 tasks

abayer mentioned this pull request Apr 14, 2022

[TEP-0100] Implementation for embedded TaskRun and Run statuses in PipelineRuns #4739

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch ApplyTaskResultsToPipelineResults to not use status maps #4753

Switch ApplyTaskResultsToPipelineResults to not use status maps #4753

abayer commented Apr 12, 2022

abayer commented Apr 12, 2022

tekton-robot commented Apr 12, 2022

vdemeester Apr 13, 2022

tekton-robot commented Apr 13, 2022

lbernick Apr 13, 2022

abayer Apr 13, 2022

lbernick Apr 13, 2022

abayer Apr 13, 2022

tekton-robot commented Apr 13, 2022

lbernick commented Apr 13, 2022

abayer commented Apr 13, 2022

lbernick commented Apr 13, 2022

lbernick commented Apr 13, 2022

tekton-robot commented Apr 13, 2022

Switch ApplyTaskResultsToPipelineResults to not use status maps #4753

Switch ApplyTaskResultsToPipelineResults to not use status maps #4753

Conversation

abayer commented Apr 12, 2022

Changes

Submitter Checklist

Release Notes

abayer commented Apr 12, 2022

tekton-robot commented Apr 12, 2022

vdemeester Apr 13, 2022

Choose a reason for hiding this comment

tekton-robot commented Apr 13, 2022

lbernick Apr 13, 2022

Choose a reason for hiding this comment

abayer Apr 13, 2022

Choose a reason for hiding this comment

lbernick Apr 13, 2022

Choose a reason for hiding this comment

abayer Apr 13, 2022

Choose a reason for hiding this comment

tekton-robot commented Apr 13, 2022

lbernick commented Apr 13, 2022

abayer commented Apr 13, 2022

lbernick commented Apr 13, 2022

lbernick commented Apr 13, 2022

tekton-robot commented Apr 13, 2022