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

abayer · 2022-04-13T17:14:23Z

Changes

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.

/kind tep
/kind feature

Submitter Checklist

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

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-13T17:14:39Z

/assign @lbernick
/assign @vdemeester
/assign @jerop

abayer · 2022-04-13T17:47:46Z

/retest

lbernick · 2022-04-13T17:45:43Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

+	var childRefs []v1beta1.ChildStatusReference
+
+	for _, rprt := range state {
+		if rprt.ResolvedConditionChecks == nil && ((rprt.CustomTask && rprt.Run == nil) || (!rprt.CustomTask && rprt.TaskRun == nil)) {


can this conditional logic be simplified? especially since isCustomTask just returns rprt.CustomTask.

One idea is to delete this block, and below have:

if rprt.CustomTask { if rprt.Run != nil { childAPIVersion = rprt.Run.APIVersion } else if rprt.ResolvedConditionChecks == nil { continue } else { childAPIVersion = runVersion } } else { ... }

Ok, I think we still need/want that if chunk, but just for TaskRuns - ResolvedConditionChecks doesn't apply at all for Runs, so it's irrelevant there. Tweak incoming.

lbernick · 2022-04-13T17:49:18Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go

+				t.Errorf("GetTaskRunName: %s", diff.PrintWantGot(d))
+			}
+			rnFromChildRefs := getRunName(nil, childRefs, tc.ptName, testPrName)
+			if d := cmp.Diff(tc.wantTrName, rnFromChildRefs); d != "" {


would it make sense to also check the output of getRunName(runsStatus, childRefs, tc.ptName, testPrName)?

Can't hurt!

tekton-robot · 2022-04-13T17:50:24Z

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%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.9%	96.9%	-0.1

…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]>

tekton-robot · 2022-04-13T18:08:24Z

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%	92.3%	-1.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	96.9%	97.2%	0.3

lbernick

/lgtm

lbernick · 2022-04-14T13:25:07Z

@tektoncd/core-maintainers please take a look at this if you have time; this PR will unblock Andrew's work implementing TEP-0100

tekton-robot · 2022-04-14T14:48:02Z

[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

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]>

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 requested review from bobcatfish and pritidesai April 13, 2022 17:14

tekton-robot assigned jerop, lbernick and vdemeester Apr 13, 2022

lbernick reviewed Apr 13, 2022

View reviewed changes

abayer force-pushed the embedded-status-support-work branch from 88421cc to 17af0fb Compare April 13, 2022 18:05

lbernick reviewed Apr 13, 2022

View reviewed changes

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

abayer mentioned this pull request Apr 13, 2022

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

Merged

5 tasks

vdemeester approved these changes Apr 14, 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 14, 2022

tekton-robot merged commit 8859310 into tektoncd:main Apr 14, 2022

abayer mentioned this pull request Apr 14, 2022

[TEP-0100] Add new updatePipelineRunStatusFromChildRefs function #4760

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

abayer commented Apr 13, 2022

abayer commented Apr 13, 2022

abayer commented Apr 13, 2022

lbernick Apr 13, 2022

abayer Apr 13, 2022

abayer Apr 13, 2022

abayer Apr 13, 2022

lbernick Apr 13, 2022

abayer Apr 13, 2022

tekton-robot commented Apr 13, 2022

tekton-robot commented Apr 13, 2022

lbernick left a comment

lbernick commented Apr 14, 2022

tekton-robot commented Apr 14, 2022

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

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

Conversation

abayer commented Apr 13, 2022

Changes

Submitter Checklist

Release Notes

abayer commented Apr 13, 2022

abayer 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

abayer 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

tekton-robot commented Apr 13, 2022

lbernick left a comment

Choose a reason for hiding this comment

lbernick commented Apr 14, 2022

tekton-robot commented Apr 14, 2022