WhenExpressions in Finally Tasks

[jerop]

Changes

Users can guard execution of Tasks using WhenExpressions, but that
is currently not supported in Finally Tasks.

This change adds support for WhenExpressions in Finally Tasks not
only to provide efficient guarded execution but also to improve the
reusability of Tasks in Finally. The proposal is described further in
the WhenExpressions in Finally Tasks TEP.

Given we've recently added support for Results and Status in
Finally Tasks, this is an opportune time to enable WhenExpressions
in Finally Tasks.

/kind feature

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

WhenExpressions are supported in Finally Tasks not only to provide efficient guarded execution but also to improve the
reusability of `Tasks` in `Finally`

[jerop]

closes #3438

[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/resources/pipelinerunresolution.go	92.0%	92.1%	0.1

[pritidesai]

thanks @jerop, I haven't reviewed the PR yet but I would like to see e2e tests in pipelinefinally_test.go

[jerop]

thanks @jerop, I haven't reviewed the PR yet but I would like to see e2e tests in pipelinefinally_test.go

i'd added reconciler tests in pipelinerun_test.go, will add more tests in pipelinefinally_test.go, thanks @pritidesai!

/hold

[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/resources/pipelinerunresolution.go	92.0%	92.1%	0.1

[jerop]

@pritidesai I've added the tests in pipelinefinally_test.go, please take a look 🙏🏾

/hold cancel

[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/resources/pipelinerunresolution.go	92.0%	92.1%	0.1

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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

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

[jerop]

/test check-pr-has-kind-label

[bobcatfish]

So far I've reviewed docs, examples and the end to end test - planning to come back and review the rest but dont want to lose this so far :)

[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/resources/pipelinerunresolution.go	92.0%	92.1%	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/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.1%	0.1

[jerop]

/test check-pr-has-kind-label

[afrittoli]

/test check-pr-has-kind-label

[pritidesai]

@jerop GetSkippedTasks adds the finally tasks into the list of skippedTasks:

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

Lines 369 to 386 in 9850785

	func (facts *PipelineRunFacts) GetSkippedTasks() []v1beta1.SkippedTask {
	var skipped []v1beta1.SkippedTask
	for _, rprt := range facts.State {
	if rprt.Skip(facts) {
	skippedTask := v1beta1.SkippedTask{
	Name: rprt.PipelineTask.Name,
	WhenExpressions: rprt.PipelineTask.WhenExpressions,
	}
	skipped = append(skipped, skippedTask)
	}
	if rprt.IsFinallySkipped(facts) {
	skippedTask := v1beta1.SkippedTask{
	Name: rprt.PipelineTask.Name,
	}
	skipped = append(skipped, skippedTask)
	}
	}
	return skipped

Can we please set WhenExpressions for finally tasks when one or more expressions are specified?

		if rprt.IsFinallySkipped(facts) {
			skippedTask := v1beta1.SkippedTask{
				Name: rprt.PipelineTask.Name,
			}
			if rprt.PipelineTask.WhenExpressions != nil || len(rprt.PipelineTask.WhenExpressions) != 0 {
				skippedTask.WhenExpressions = rprt.PipelineTask.WhenExpressions
			}
			skipped = append(skipped, skippedTask)
		}

such that finally tasks being in the list of skipped tasks have the when expressions initialized.

    {
        "name": "finally-task-should-be-skipped-1"
      },
      {
        "name": "finally-task-should-be-skipped-2"
      },
      {
        "name": "finally-task-should-be-skipped-3"
      }

After these changes:

     {
        "name": "finally-task-should-be-skipped-1",
        "whenExpressions": [
          {
            "input": "Succeeded",
            "operator": "in",
            "values": [
              "Failure"
            ]
          }
        ]
      },
      {
        "name": "finally-task-should-be-skipped-2",
        "whenExpressions": [
          {
            "input": "yes",
            "operator": "in",
            "values": [
              "missing"
            ]
          }
        ]
      },
      {
        "name": "finally-task-should-be-skipped-3",
        "whenExpressions": [
          {
            "input": "README.md",
            "operator": "notin",
            "values": [
              "README.md"
            ]
          }
        ]
      }

[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/resources/pipelinerunresolution.go	92.0%	92.1%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	69.5%	69.8%	0.4

[jerop]

thanks for the review @pritidesai! 🙏🏾

Can we please set WhenExpressions for finally tasks when one or more expressions are specified?

		if rprt.IsFinallySkipped(facts) {
			skippedTask := v1beta1.SkippedTask{
				Name: rprt.PipelineTask.Name,
			}
			if rprt.PipelineTask.WhenExpressions != nil || len(rprt.PipelineTask.WhenExpressions) != 0 {
				skippedTask.WhenExpressions = rprt.PipelineTask.WhenExpressions
			}
			skipped = append(skipped, skippedTask)
		}

makes sense to do it similarly to Tasks, but did it a bit differently to ensure we don't include WhenExpressions in Finally Tasks that were skipped because they referenced missing Results (not because their WhenExpressions evaluated to False):

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

Lines 379 to 389 in c719b6a

	if rprt.IsFinallySkipped(facts) {
	skippedTask := v1beta1.SkippedTask{
	Name: rprt.PipelineTask.Name,
	}
	// include the when expressions only when the finally task was skipped because
	// its when expressions evaluated to false (not because results variables were missing)
	if !rprt.PipelineTask.WhenExpressions.HaveVariables() {
	skippedTask.WhenExpressions = rprt.PipelineTask.WhenExpressions
	}
	skipped = append(skipped, skippedTask)
	}

[pritidesai]

thanks a bunch @jerop, great point 🙇‍♀️

[pritidesai]

Thanks a bunch @jerop for this feature.

I am having hard time understanding validation unit tests. I feel we have many overlapping tests. Ideally, would like to see the separate unit tests validating when expression syntax e.g. input and values are initialized, a valid operator is specified, etc. Now once we have validated the syntax, I don't see a need to have separate tests to validate the syntax again as part of either tasks or finally. Rather validate when expressions references (e.g. referencing a task result or defined parameter) with the tasks and finally sections. This can be refactored and simplified later and will not block this PR.

/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/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.6%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.0%	92.1%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	69.5%	69.8%	0.4

[jerop]

@pritidesai I agree, we need to refactor the validation testing -- can open an issue to track that

[pritidesai]

/lgtm

[pritidesai]

hey @jerop, based on my understanding, rprt.Skip(pipelineRunFacts) returns false for any finally task but rprt.IsFinallySkipped(pipelineRunFacts) evaluates when expression and returns true since the when expression evaluates to false.

[pritidesai]

but this would work, provided nextRprts contains the finally tasks 🤣