Add check for skipping in When Expressions with Result variable replacement

[jerop]

Changes

tasks with when expressions using variables that evaluated to false were mistakenly executed because it didn't have an additional check to validate that the task should be skipped after task result variable replacement

the point where we excluded the tasks with when expressions containing static inputs or parameter variables (which are replaced earlier as read from the pipeline) is here:

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

Lines 260 to 272 in 079a6c8

	// SuccessfulOrSkippedDAGTasks returns a list of the names of all of the PipelineTasks in state
	// which have successfully completed or skipped
	func (state PipelineRunState) SuccessfulOrSkippedDAGTasks(d *dag.Graph) []string {
	tasks := []string{}
	for _, t := range state {
	if isTaskInGraph(t.PipelineTask.Name, d) {
	if t.IsSuccessful() || t.Skip(state, d) {
	tasks = append(tasks, t.PipelineTask.Name)
	}
	}
	}
	return tasks
	}

at that point, when expressions containing results don't have the variables replaced so they are not excluded -- that function is used here:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Lines 499 to 511 in 9064142

	// when pipeline run is stopping, do not schedule any new task and only
	// wait for all running tasks to complete and report their status
	if !pipelineState.IsStopping(d) {
	// candidateTasks is initialized to DAG root nodes to start pipeline execution
	// candidateTasks is derived based on successfully finished tasks and/or skipped tasks
	candidateTasks, err := dag.GetSchedulable(d, pipelineState.SuccessfulOrSkippedDAGTasks(d)...)
	if err != nil {
	logger.Errorf("Error getting potential next tasks for valid pipelinerun %s: %v", pr.Name, err)
	return controller.NewPermanentError(err)
	}
	// nextRprts holds a list of pipeline tasks which should be executed next
	nextRprts = pipelineState.GetNextTasks(candidateTasks)
	}

the task results are applied later, right after which we add this additional check to exclude the when expressions containing results:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Lines 522 to 527 in 9064142

	resources.ApplyTaskResults(nextRprts, resolvedResultRefs)
	for _, rprt := range nextRprts {
	if rprt == nil || rprt.Skip(pipelineState, d) {
	continue
	}

Fixes #3188

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

`Tasks` with `WhenExpressions` using variable replacements are not executed when the `WhenExpressions` evaluate to false

[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/pipelinerun/pipelinerun.go	86.5%	86.8%	0.2

[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/pipelinerun/pipelinerun.go	86.5%	86.8%	0.2

[bobcatfish]

Can you confirm that the example you updated fails without your change?

I also want to check, #3196 implies this is finally related but it sounds like that's a red herring and it's actually related to variable replacement?

(I'm wondering also if makes sense to have an example/test that verifies that finally works properly with this as well)

[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/pipelinerun/pipelinerun.go	86.5%	86.8%	0.2

[pritidesai]

@jerop, please add a unit test in pipelinerun_test.go making sure the task is skipped and no task run getting created. I just modified an existing unit test to not match the task result value (resulting when expression to false) and that task was skipped but task run was created as well.

I am trying to reproduce it through the integration test from examples.

[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/pipelinerun/pipelinerun.go	86.5%	86.8%	0.2

[jerop]

@pritidesai thank you for the review!

added a task to the when expressions example that reproduced the issue, and also added checks in the unit tests to ensure the taskruns are not created when variable replacement is used -- please have a look when you have a chance

[pritidesai]

/kind bug

[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/pipelinerun/pipelinerun.go	86.5%	86.8%	0.2

[bobcatfish]

I agree with @pritidesai about refactoring the code a bit - and @jerop with your analysis of trying to find a way to only have to check once so we dont have to worry about checking for skipped tasks correctly in two different places!

but let's tackle that after we get this fixed :D

/lgtm
/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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

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