consuming task results in finally

[pritidesai]

Changes

Final tasks can be configured to consume results of PipelineTasks from tasks section.

Based on the TEP, a final task failing to resolve task results is added to the list of skippedTasks and warning is recorded in controller logs.

unable to find result referenced by param "param1" in "final-task" ...

Implements TEP #0004
Closes #2557

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices
• Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Final tasks can be configured to consume `results` of `PipelineTasks` from `tasks` section. 

[pritidesai]

/kind feature

[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/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.3%	-0.1
pkg/reconciler/pipeline/dag/dag.go	98.8%	98.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	73.0%	-2.5

[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/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.3%	-0.1
pkg/reconciler/pipeline/dag/dag.go	98.8%	98.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	85.2%	9.8

[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/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.3%	-0.1
pkg/reconciler/pipeline/dag/dag.go	98.8%	98.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	85.2%	9.8

[jerop]

excited to see results supported in finally tasks!

we can look into extending it to support guarding execution of final tasks based on task results using when expressions

[jerop]

I think it might be worthwhile to explore adding these final tasks to skipped tasks (or another separate section for final tasks with invalid results) -- instead of creating a TaskRun object for a final task that wasn't executed

related discussion: #3139

[pritidesai]

yes thats an alternative, another alternative could be: Tekton controller replacing such unresolved param with type default which is empty string for string type params. Let that final task execute expecting that the task author has implemented sanitization on such input params. @bobcatfish thoughts?

[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/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.3%	-0.1
pkg/reconciler/pipeline/dag/dag.go	98.8%	98.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	85.2%	9.8

[vdemeester]

/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/pipeline/dag/dag.go	98.8%	98.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	75.4%	85.2%	9.8

[anderoo]

Happy to see this PR. Moves us closer to being able to conditionally execute finally tasks.

[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/pipeline/dag/dag.go	98.8%	98.9%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	72.1%	81.2%	9.1

[pritidesai]

Please review whenever you can @tektoncd/core-maintainers, would like to get this in 0.17 🙏

[ghost]

Small suggestion, doesn't have to be a blocker on this PR, but it would be great to see an example yaml added to examples/v1beta1/pipelineruns for this too if possible.

/approve

[pritidesai]

thanks @sbwsg, thats a great suggestion. Putting it on a brief hold until I add and end-to-end example.
/hold

[pritidesai]

Hey @bobcatfish I think I have responded to most of the comments. I have tried to explain both of these:

(1) Need for a separate isFinallySkipped function and possible refactoring. Can be avoided by introducing a list of skippedTasks as part of pipelineRunFacts.
(2) Checking if a finally task must be skipped in one single i.e. instead of resolving a task result, check the reference task status, if failed or skipped, skip finally

Let me know if you would want to see any more changes in this PR including any refactoring. I will be happy to change this PR or create separate PR if possible.

Also, simplifying the check if a finally should be skipped only by looking at the referenced task status is not reliable. Even after a referenced task exits with success, it is possible that referenced result might not be initialized. These simplification would work after we fix this issue #3497 and adding this kind of validation.

[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/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0
pkg/reconciler/pipelinerun/pipelinerun.go	82.6%	82.9%	0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.3%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	64.1%	67.5%	3.5
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.0%	91.2%	0.2

[pritidesai]

one more ping 🛎
🙏🏻 Would like to get this in if possible 🙏🏻

[pritidesai]

/lgtm cancel
/hold

Hey @vdemeester we discussed this in the planning meeting this morning. I think we put this PR on hold so that it can be redesigned (skip instead of creating an object reference for a finally task referring to missing task results). Can we cancel the hold since the design is updated?

We have moved this PR to next milestone 0.21 for now.

[vdemeester]

/lgtm cancel
/hold

Hey @vdemeester we discussed this in the planning meeting this morning. I think we put this PR on hold so that it can be redesigned (skip instead of creating an object reference for a finally task referring to missing task results). Can we cancel the hold since the design is updated?

We have moved this PR to next milestone 0.21 for now.

👍🏼

/hold cancel

[vdemeester]

@pritidesai needs a rebase 🙏🏼

[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/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0
pkg/reconciler/pipelinerun/pipelinerun.go	82.6%	82.8%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	91.7%	92.0%	0.2
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	66.3%	69.5%	3.2
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	91.3%	91.6%	0.3

[pritidesai]

@pritidesai needs a rebase 🙏🏼

sorry @vdemeester for delay, done 👍

[vdemeester]

/lgtm