diff --git a/config/config-feature-flags.yaml b/config/config-feature-flags.yaml index 9ab11ef6e4b..870ceadd409 100644 --- a/config/config-feature-flags.yaml +++ b/config/config-feature-flags.yaml @@ -82,3 +82,6 @@ data: # Setting this flag will determine which gated features are enabled. # Acceptable values are "stable" or "alpha". enable-api-fields: "stable" + # Setting this flag to "true" scopes when expressions to guard a Task only + # instead of a Task and its dependent Tasks. + scope-when-expressions-to-task: "false" diff --git a/docs/deprecations.md b/docs/deprecations.md index 0e344d3638c..0b949e7fe78 100644 --- a/docs/deprecations.md +++ b/docs/deprecations.md @@ -25,3 +25,5 @@ being deprecated. | [`Conditions` CRD is deprecated and will be removed. Use `when` expressions instead.](https://github.com/tektoncd/community/blob/main/teps/0007-conditions-beta.md) | [v0.16.0](https://github.com/tektoncd/pipeline/releases/tag/v0.16.0) | Alpha | Nov 02 2020 | | [The `disable-home-env-overwrite` flag will be removed](https://github.com/tektoncd/pipeline/issues/2013) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | Beta | February 10 2022 | | [The `disable-working-dir-overwrite` flag will be removed](https://github.com/tektoncd/pipeline/issues/1836) | [v0.24.0](https://github.com/tektoncd/pipeline/releases/tag/v0.24.0) | Beta | February 10 2022 | +| [The `scope-when-expressions-to-task` flag will be flipped from "false" to "true"](https://github.com/tektoncd/pipeline/issues/1836) | [v0.27.0](https://github.com/tektoncd/pipeline/releases/tag/v0.27.0) | Beta | February 10 2022 | +| [The `scope-when-expressions-to-task` flag will be removed](https://github.com/tektoncd/pipeline/issues/1836) | [v0.27.0](https://github.com/tektoncd/pipeline/releases/tag/v0.27.0) | Beta | March 10 2022 | diff --git a/docs/install.md b/docs/install.md index 5ca60d67f56..f09487f914b 100644 --- a/docs/install.md +++ b/docs/install.md @@ -366,6 +366,10 @@ use of custom tasks in pipelines. most stable features to be used. Set it to "alpha" to allow alpha features to be used. +- `scope-when-expressions-to-task`: set this flag to "true" to scope `when` expressions to guard a `Task` only. Set it + to "false" to guard a `Task` and its dependent `Tasks`. It defaults to "false". For more information, see [guarding + `Task` execution using `when` expressions](pipelines.md#guard-task-execution-using-whenexpressions). + For example: ```yaml diff --git a/docs/pipelines.md b/docs/pipelines.md index 64abfa5125c..855cdd9aac2 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -474,6 +474,226 @@ There are a lot of scenarios where `when` expressions can be really useful. Some - Checking if the name of a CI job matches - Checking if an optional Workspace has been provided +#### Guarding a `Task` and its dependent `Tasks` + +When `when` expressions evaluate to `False`, the `Task` and its dependent `Tasks` will be skipped by default while the +rest of the `Pipeline` will execute. Dependencies between `Tasks` can be either ordering ([`runAfter`](https://github.com/tektoncd/pipeline/blob/main/docs/pipelines.md#using-the-runafter-parameter)) +or resource (e.g. [`Results`](https://github.com/tektoncd/pipeline/blob/main/docs/pipelines.md#using-results)) +dependencies, as further described in [configuring execution order](#configuring-the-task-execution-order). The global +default scope of `when` expressions is set to a `Task` and its dependent`Tasks`; `scope-when-expressions-to-task` field +in [`config/config-feature-flags.yaml`](install.md#customizing-the-pipelines-controller-behavior) defaults to "false". + +**Note:** Scoping `when` expressions to a `Task` and its dependent `Tasks` is deprecated + +To guard a `Task` and its dependent Tasks: +- cascade the `when` expressions to the specific dependent `Tasks` to be guarded as well +- compose the `Task` and its dependent `Tasks` as a unit to be guarded and executed together using `Pipelines` in `Pipelines` + +##### Cascade `when` expressions to the specific dependent `Tasks` + +Pick and choose which specific dependent `Tasks` to guard as well, and cascade the `when` expressions to those `Tasks`. + +Taking the use case below, a user who wants to guard `manual-approval` and its dependent `Tasks`: + +``` + tests + | + v + manual-approval + | | + v (approver) + build-image | + | v + v slack-msg + deploy-image +``` + +The user can design the `Pipeline` to solve their use case as such: + +```yaml +tasks: +... +- name: manual-approval + runAfter: + - tests + when: + - input: $(params.git-action) + operator: in + values: + - merge + taskRef: + name: manual-approval + +- name: build-image + when: + - input: $(params.git-action) + operator: in + values: + - merge + runAfter: + - manual-approval + taskRef: + name: build-image + +- name: deploy-image + when: + - input: $(params.git-action) + operator: in + values: + - merge + runAfter: + - build-image + taskRef: + name: deploy-image + +- name: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg +``` + +##### Compose using Pipelines in Pipelines + +Compose a set of `Tasks` as a unit of execution using `Pipelines` in `Pipelines`, which allows for guarding a `Task` and +its dependent `Tasks` (as a sub-`Pipeline`) using `when` expressions. + +**Note:** `Pipelines` in `Pipelines` is an [experimental feature](https://github.com/tektoncd/experimental/tree/main/pipelines-in-pipelines) + +Taking the use case below, a user who wants to guard `manual-approval` and its dependent `Tasks`: + +``` + tests + | + v + manual-approval + | | + v (approver) + build-image | + | v + v slack-msg + deploy-image +``` + +The user can design the `Pipelines` to solve their use case as such: + +```yaml +## sub pipeline (approve-build-deploy-slack) +tasks: + - name: manual-approval + runAfter: + - integration-tests + taskRef: + name: manual-approval + + - name: build-image + runAfter: + - manual-approval + taskRef: + name: build-image + + - name: deploy-image + runAfter: + - build-image + taskRef: + name: deploy-image + + - name: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg + +--- +## main pipeline +tasks: +... +- name: approve-build-deploy-slack + runAfter: + - tests + when: + - input: $(params.git-action) + operator: in + values: + - merge + taskRef: + apiVersion: tekton.dev/v1beta1 + kind: Pipeline + name: approve-build-deploy-slack +``` + +#### Guarding a `Task` only + +To guard a `Task` only and unblock execution of its dependent `Tasks`, set the global default scope of `when` expressions +to `Task` using the `scope-when-expressions-to-task` field in [`config/config-feature-flags.yaml`](install.md#customizing-the-pipelines-controller-behavior) +by changing it to "true" +- The ordering-dependent `Tasks` will be executed +- The resource-dependent `Tasks` (and their dependencies) will be skipped because of missing `Results` from the skipped + parent `Task`. When we add support for [default `Results`](https://github.com/tektoncd/community/pull/240), then the + resource-dependent `Tasks` may be executed if the default `Results` from the skipped parent `Task` are specified. In + addition, if a resource-dependent `Task` needs a file from a guarded parent `Task` in a shared `Workspace`, make sure + to handle the execution of the child `Task` in case the expected file is missing from the `Workspace` because the + guarded parent `Task` is skipped. + +``` + tests + | + v + manual-approval + | | + v (approver) + build-image | + | v + v slack-msg + deploy-image +``` + +Taking the use case above, a user who wants to guard `manual-approval` only can design the `Pipeline` as such: + +```yaml +tasks: +... +- name: manual-approval + runAfter: + - tests + when: + - input: $(params.git-action) + operator: in + values: + - merge + taskRef: + name: manual-approval + +- name: build-image + runAfter: + - manual-approval + taskRef: + name: build-image + +- name: deploy-image + runAfter: + - build-image + taskRef: + name: deploy-image + +- name: slack-msg + params: + - name: approver + value: $(tasks.manual-approval.results.approver) + taskRef: + name: slack-msg +``` + +With `when` expressions scoped to `Task`, if `manual-approval` is skipped, execution of it's dependent `Tasks` +(`slack-msg`, `build-image` and `deploy-image`) would be unblocked regardless: +- `build-image` and `deploy-image` should be executed successfully +- `slack-msg` will be skipped because it is missing the `approver` `Result` from `manual-approval` + - dependents of `slack-msg` would have been skipped too if it had any of them + - if `manual-approval` specifies a default `approver` `Result`, such as "None", then `slack-msg` would be executed + ([supporting default `Results` is in progress](https://github.com/tektoncd/community/pull/240)) + ### Guard `Task` execution using `Conditions` **Note:** `Conditions` are [deprecated](./deprecations.md), use [`when` expressions](#guard-task-execution-using-when-expressions) instead. @@ -700,10 +920,13 @@ so that one will run before another and the execution of the `Pipeline` progress without getting stuck in an infinite loop. This is done using: - -- [`from`](#using-the-from-parameter) clauses on the [`PipelineResources`](resources.md) used by each `Task` -- [`runAfter`](#using-the-runafter-parameter) clauses on the corresponding `Tasks` -- By linking the [`results`](#configuring-execution-results-at-the-pipeline-level) of one `Task` to the params of another +- _resource dependencies_: + - [`from`](#using-the-from-parameter) clauses on the [`PipelineResources`](resources.md) used by each `Task` + - [`results`](#configuring-execution-results-at-the-pipeline-level) of one `Task` being pa `params` or + `when` expressions of another + +- _ordering dependencies_: + - [`runAfter`](#using-the-runafter-parameter) clauses on the corresponding `Tasks` For example, the `Pipeline` defined as follows diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go index 5073ea5d169..639c37148b5 100644 --- a/pkg/apis/config/feature_flags.go +++ b/pkg/apis/config/feature_flags.go @@ -37,6 +37,7 @@ const ( enableTektonOCIBundles = "enable-tekton-oci-bundles" enableCustomTasks = "enable-custom-tasks" enableAPIFields = "enable-api-fields" + scopeWhenExpressionsToTask = "scope-when-expressions-to-task" DefaultDisableHomeEnvOverwrite = true DefaultDisableWorkingDirOverwrite = true DefaultDisableAffinityAssistant = false @@ -45,6 +46,7 @@ const ( DefaultRequireGitSSHSecretKnownHosts = false DefaultEnableTektonOciBundles = false DefaultEnableCustomTasks = false + DefaultScopeWhenExpressionsToTask = false DefaultEnableAPIFields = StableAPIFields ) @@ -59,6 +61,7 @@ type FeatureFlags struct { RequireGitSSHSecretKnownHosts bool EnableTektonOCIBundles bool EnableCustomTasks bool + ScopeWhenExpressionsToTask bool EnableAPIFields string } @@ -105,6 +108,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { if err := setFeature(requireGitSSHSecretKnownHostsKey, DefaultRequireGitSSHSecretKnownHosts, &tc.RequireGitSSHSecretKnownHosts); err != nil { return nil, err } + if err := setFeature(scopeWhenExpressionsToTask, DefaultScopeWhenExpressionsToTask, &tc.ScopeWhenExpressionsToTask); err != nil { + return nil, err + } if err := setEnabledAPIFields(cfgMap, DefaultEnableAPIFields, &tc.EnableAPIFields); err != nil { return nil, err } diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go index 752069c6436..6fb7909e6b4 100644 --- a/pkg/apis/config/feature_flags_test.go +++ b/pkg/apis/config/feature_flags_test.go @@ -38,6 +38,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { DisableHomeEnvOverwrite: false, DisableWorkingDirOverwrite: false, RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, EnableAPIFields: "stable", }, fileName: config.GetFeatureFlagsConfigName(), @@ -51,6 +52,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { RequireGitSSHSecretKnownHosts: true, EnableTektonOCIBundles: true, EnableCustomTasks: true, + ScopeWhenExpressionsToTask: true, EnableAPIFields: "alpha", }, fileName: "feature-flags-all-flags-set", @@ -66,6 +68,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { DisableHomeEnvOverwrite: true, DisableWorkingDirOverwrite: true, RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, }, fileName: "feature-flags-enable-api-fields-overrides-bundles-and-custom-tasks", }, @@ -78,6 +81,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) { DisableHomeEnvOverwrite: true, DisableWorkingDirOverwrite: true, RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, }, fileName: "feature-flags-bundles-and-custom-tasks", }, @@ -98,6 +102,7 @@ func TestNewFeatureFlagsFromEmptyConfigMap(t *testing.T) { DisableHomeEnvOverwrite: true, DisableWorkingDirOverwrite: true, RunningInEnvWithInjectedSidecars: true, + ScopeWhenExpressionsToTask: config.DefaultScopeWhenExpressionsToTask, EnableAPIFields: "stable", } verifyConfigFileWithExpectedFeatureFlagsConfig(t, FeatureFlagsConfigEmptyName, expectedConfig) @@ -141,6 +146,8 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) { fileName: "feature-flags-invalid-boolean", }, { fileName: "feature-flags-invalid-enable-api-fields", + }, { + fileName: "feature-flags-invalid-scope-when-expressions-to-task", }} { t.Run(tc.fileName, func(t *testing.T) { cm := test.ConfigMapFromTestFile(t, tc.fileName) diff --git a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml index e56766124ce..6e5e4958e51 100644 --- a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml +++ b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml @@ -25,4 +25,5 @@ data: require-git-ssh-secret-known-hosts: "true" enable-tekton-oci-bundles: "true" enable-custom-tasks: "true" + scope-when-expressions-to-task: "true" enable-api-fields: "alpha" diff --git a/pkg/apis/config/testdata/feature-flags-invalid-scope-when-expressions-to-task.yaml b/pkg/apis/config/testdata/feature-flags-invalid-scope-when-expressions-to-task.yaml new file mode 100644 index 00000000000..54569ae7cba --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags-invalid-scope-when-expressions-to-task.yaml @@ -0,0 +1,21 @@ +# Copyright 2021 The Tekton Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: feature-flags + namespace: tekton-pipelines +data: + scope-when-expressions-to-task: "im-not-a-boolean" diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 4992af282a2..8d5574bbf3a 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -482,10 +482,11 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get // Build PipelineRunFacts with a list of resolved pipeline tasks, // dag tasks graph and final tasks graph pipelineRunFacts := &resources.PipelineRunFacts{ - State: pipelineRunState, - SpecStatus: pr.Spec.Status, - TasksGraph: d, - FinalTasksGraph: dfinally, + State: pipelineRunState, + SpecStatus: pr.Spec.Status, + TasksGraph: d, + FinalTasksGraph: dfinally, + ScopeWhenExpressionsToTask: config.FromContextOrDefaults(ctx).FeatureFlags.ScopeWhenExpressionsToTask, } for _, rprt := range pipelineRunFacts.State { @@ -634,7 +635,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip return controller.NewPermanentError(err) } - resolvedResultRefs, err := resources.ResolveResultRefs(pipelineRunFacts.State, nextRprts) + resolvedResultRefs, _, err := resources.ResolveResultRefs(pipelineRunFacts.State, nextRprts) if err != nil { logger.Infof("Failed to resolve task result reference for %q with error %v", pr.Name, err) pr.Status.MarkFailed(ReasonInvalidTaskResultReference, err.Error()) @@ -655,7 +656,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip // Before creating TaskRun for scheduled final task, check if it's consuming a task result // Resolve and apply task result wherever applicable, report warning in case resolution fails for _, rprt := range fnextRprts { - resolvedResultRefs, err := resources.ResolveResultRef(pipelineRunFacts.State, rprt) + resolvedResultRefs, _, err := resources.ResolveResultRef(pipelineRunFacts.State, rprt) if err != nil { logger.Infof("Final task %q is not executed as it could not resolve task params for %q: %v", rprt.PipelineTask.Name, pr.Name, err) continue @@ -666,7 +667,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip } for _, rprt := range nextRprts { - if rprt == nil || rprt.Skip(pipelineRunFacts) || rprt.IsFinallySkipped(pipelineRunFacts) { + if rprt == nil || rprt.Skip(pipelineRunFacts).IsSkipped || rprt.IsFinallySkipped(pipelineRunFacts).IsSkipped { continue } if rprt.ResolvedConditionChecks == nil || rprt.ResolvedConditionChecks.IsSuccess() { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 80c45e73181..31474920d5f 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -3797,6 +3797,299 @@ func TestReconcileWithWhenExpressionsWithTaskResults(t *testing.T) { } } +func TestReconcileWithWhenExpressionsScopedToTask(t *testing.T) { + // (b) + // / + // (a) ———— (c) ———— (d) + // \ + // (e) ———— (f) + names.TestingSeed() + ps := []*v1beta1.Pipeline{tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec( + // a-task is skipped because its when expressions evaluate to false + tb.PipelineTask("a-task", "a-task", + tb.PipelineTaskWhenExpression("foo", selection.In, []string{"bar"}), + ), + // b-task is executed regardless of running after skipped a-task because when expressions are scoped to task + tb.PipelineTask("b-task", "b-task", + tb.RunAfter("a-task"), + ), + // c-task is skipped because its when expressions evaluate to false (not because it's parent a-task is skipped) + tb.PipelineTask("c-task", "c-task", + tb.PipelineTaskWhenExpression("foo", selection.In, []string{"bar"}), + tb.RunAfter("a-task"), + ), + // d-task is executed regardless of running after skipped parent c-task (and skipped grandparent a-task) + // because when expressions are scoped to task + tb.PipelineTask("d-task", "d-task", + tb.RunAfter("c-task"), + ), + // e-task is attempted regardless of running after skipped a-task because when expressions are scoped to task + // but then get skipped because of missing result references from a-task + tb.PipelineTask("e-task", "e-task", + tb.PipelineTaskWhenExpression("$(tasks.a-task.results.aResult)", selection.In, []string{"aResultValue"}), + ), + // f-task is skipped because its parent task e-task is skipped because of missing result reference from a-task + tb.PipelineTask("f-task", "f-task", + tb.RunAfter("e-task"), + ), + ))} + prs := []*v1beta1.PipelineRun{tb.PipelineRun("test-pipeline-run-different-service-accs", tb.PipelineRunNamespace("foo"), + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunServiceAccountName("test-sa-0"), + ), + )} + // initialize the pipelinerun with the skipped a-task + prs[0].Status.SkippedTasks = append(prs[0].Status.SkippedTasks, v1beta1.SkippedTask{ + Name: "a-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "foo", + Operator: selection.In, + Values: []string{"bar"}, + }}, + }) + // initialize the tasks used in the pipeline + ts := []*v1beta1.Task{ + tb.Task("a-task", tb.TaskNamespace("foo"), + tb.TaskSpec(tb.TaskResults("aResult", "a result")), + ), + tb.Task("b-task", tb.TaskNamespace("foo")), + tb.Task("c-task", tb.TaskNamespace("foo")), + tb.Task("d-task", tb.TaskNamespace("foo")), + tb.Task("e-task", tb.TaskNamespace("foo")), + tb.Task("f-task", tb.TaskNamespace("foo")), + } + + // set the scope of when expressions to task -- execution of dependent tasks is unblocked + cms := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "scope-when-expressions-to-task": "true", + }, + }, + } + + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + Tasks: ts, + ConfigMaps: cms, + } + prt := newPipelineRunTest(d, t) + defer prt.Cancel() + + wantEvents := []string{ + "Normal Started", + "Normal Running Tasks Completed: 0 \\(Failed: 0, Cancelled 0\\), Incomplete: 2, Skipped: 4", + } + pipelineRun, clients := prt.reconcileRun("foo", "test-pipeline-run-different-service-accs", wantEvents, false) + + taskRunExists := func(taskName string, taskRunName string) { + expectedTaskRun := tb.TaskRun(taskRunName, + tb.TaskRunNamespace("foo"), + tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-different-service-accs", + tb.OwnerReferenceAPIVersion("tekton.dev/v1beta1"), + tb.Controller, tb.BlockOwnerDeletion, + ), + tb.TaskRunLabel("tekton.dev/memberOf", "tasks"), + tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline"), + tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-different-service-accs"), + tb.TaskRunLabel("tekton.dev/pipelineTask", taskName), + tb.TaskRunSpec( + tb.TaskRunTaskRef(taskName), + tb.TaskRunServiceAccountName("test-sa-0"), + ), + ) + + actual, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", taskName), + Limit: 1, + }) + + if err != nil { + t.Fatalf("Failure to list TaskRuns %s", err) + } + if len(actual.Items) != 1 { + t.Fatalf("Expected 1 TaskRun got %d", len(actual.Items)) + } + actualTaskRun := actual.Items[0] + if d := cmp.Diff(&actualTaskRun, expectedTaskRun, ignoreResourceVersion); d != "" { + t.Errorf("expected to see TaskRun %v created. Diff %s", taskRunName, diff.PrintWantGot(d)) + } + } + + taskRunExists("b-task", "test-pipeline-run-different-service-accs-b-task-mz4c7") + taskRunExists("d-task", "test-pipeline-run-different-service-accs-d-task-78c5n") + + actualSkippedTasks := pipelineRun.Status.SkippedTasks + expectedSkippedTasks := []v1beta1.SkippedTask{{ + // its when expressions evaluate to false + Name: "a-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "foo", + Operator: "in", + Values: []string{"bar"}, + }}, + }, { + // its when expressions evaluate to false + Name: "c-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "foo", + Operator: "in", + Values: []string{"bar"}, + }}, + }, { + // was attempted, but has missing results references + Name: "e-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "$(tasks.a-task.results.aResult)", + Operator: "in", + Values: []string{"aResultValue"}, + }}, + }, { + Name: "f-task", + }} + if d := cmp.Diff(expectedSkippedTasks, actualSkippedTasks); d != "" { + t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d)) + } + + // confirm that there are no taskruns created for the skipped tasks + skippedTasks := []string{"a-task", "c-task", "e-task", "f-task"} + for _, skippedTask := range skippedTasks { + labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", skippedTask) + actualSkippedTask, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: labelSelector, + Limit: 1, + }) + if err != nil { + t.Fatalf("Failure to list TaskRun's %s", err) + } + if len(actualSkippedTask.Items) != 0 { + t.Fatalf("Expected 0 TaskRuns got %d", len(actualSkippedTask.Items)) + } + } +} + +func TestReconcileWithWhenExpressionsScopedToTaskWitResultRefs(t *testing.T) { + names.TestingSeed() + ps := []*v1beta1.Pipeline{tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec( + // a-task is executed and produces a result aResult with value aResultValue + tb.PipelineTask("a-task", "a-task"), + // b-task is skipped because it has when expressions, with result reference to a-task, that evaluate to false + tb.PipelineTask("b-task", "b-task", + tb.PipelineTaskWhenExpression("$(tasks.a-task.results.aResult)", selection.In, []string{"notResultValue"}), + ), + // c-task is executed regardless of running after skipped b-task because when expressions are scoped to task + tb.PipelineTask("c-task", "c-task", + tb.RunAfter("b-task"), + ), + ))} + prs := []*v1beta1.PipelineRun{tb.PipelineRun("test-pipeline-run-different-service-accs", tb.PipelineRunNamespace("foo"), + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunServiceAccountName("test-sa-0"), + ), + )} + ts := []*v1beta1.Task{ + tb.Task("a-task", tb.TaskNamespace("foo"), + tb.TaskSpec(tb.TaskResults("aResult", "a result")), + ), + tb.Task("b-task", tb.TaskNamespace("foo")), + tb.Task("c-task", tb.TaskNamespace("foo")), + } + trs := []*v1beta1.TaskRun{ + tb.TaskRun("test-pipeline-run-different-service-accs-a-task-xxyyy", + tb.TaskRunNamespace("foo"), + tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-different-service-accs", + tb.OwnerReferenceAPIVersion("tekton.dev/v1beta1"), + tb.Controller, tb.BlockOwnerDeletion, + ), + tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline"), + tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-different-service-accs"), + tb.TaskRunLabel("tekton.dev/pipelineTask", "a-task"), + tb.TaskRunSpec( + tb.TaskRunTaskRef("hello-world"), + tb.TaskRunServiceAccountName("test-sa"), + ), + tb.TaskRunStatus( + tb.StatusCondition( + apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + ), + tb.TaskRunResult("aResult", "aResultValue"), + ), + ), + } + // set the scope of when expressions to task -- execution of dependent tasks is unblocked + cms := []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()}, + Data: map[string]string{ + "scope-when-expressions-to-task": "true", + }, + }, + } + + d := test.Data{ + PipelineRuns: prs, + Pipelines: ps, + Tasks: ts, + TaskRuns: trs, + ConfigMaps: cms, + } + prt := newPipelineRunTest(d, t) + defer prt.Cancel() + + wantEvents := []string{ + "Normal Started", + "Normal Running Tasks Completed: 1 \\(Failed: 0, Cancelled 0\\), Incomplete: 1, Skipped: 1", + } + pipelineRun, clients := prt.reconcileRun("foo", "test-pipeline-run-different-service-accs", wantEvents, false) + + actual, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: "tekton.dev/pipelineTask=c-task,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", + Limit: 1, + }) + + if err != nil { + t.Fatalf("Failure to list TaskRuns %s", err) + } + if len(actual.Items) != 1 { + t.Fatalf("Expected 1 TaskRun got %d", len(actual.Items)) + } + + actualSkippedTasks := pipelineRun.Status.SkippedTasks + expectedSkippedTasks := []v1beta1.SkippedTask{{ + // its when expressions evaluate to false + Name: "b-task", + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "aResultValue", + Operator: "in", + Values: []string{"notResultValue"}, + }}, + }} + if d := cmp.Diff(expectedSkippedTasks, actualSkippedTasks); d != "" { + t.Errorf("expected to find Skipped Tasks %v. Diff %s", expectedSkippedTasks, diff.PrintWantGot(d)) + } + + // confirm that there are no taskruns created for the skipped tasks + skippedTasks := []string{"b-task"} + for _, skippedTask := range skippedTasks { + labelSelector := fmt.Sprintf("tekton.dev/pipelineTask=%s,tekton.dev/pipelineRun=test-pipeline-run-different-service-accs", skippedTask) + actualSkippedTask, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").List(prt.TestAssets.Ctx, metav1.ListOptions{ + LabelSelector: labelSelector, + Limit: 1, + }) + if err != nil { + t.Fatalf("Failure to list TaskRun's %s", err) + } + if len(actualSkippedTask.Items) != 0 { + t.Fatalf("Expected 0 TaskRuns got %d", len(actualSkippedTask.Items)) + } + } +} + // TestReconcileWithAffinityAssistantStatefulSet tests that given a pipelineRun with workspaces, // an Affinity Assistant StatefulSet is created for each PVC workspace and // that the Affinity Assistant names is propagated to TaskRuns. diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 9210086080e..4b14536e0c3 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -39,6 +39,24 @@ const ( ReasonConditionCheckFailed = "ConditionCheckFailed" ) +type SkippingReason string + +const ( + WhenExpressionsSkip SkippingReason = "WhenExpressionsSkip" + ConditionsSkip SkippingReason = "ConditionsSkip" + ParentTasksSkip SkippingReason = "ParentTasksSkip" + IsStoppingSkip SkippingReason = "IsStoppingSkip" + IsGracefullyCancelledSkip SkippingReason = "IsGracefullyCancelledSkip" + IsGracefullyStoppedSkip SkippingReason = "IsGracefullyStoppedSkip" + MissingResultsSkip SkippingReason = "MissingResultsSkip" + None SkippingReason = "None" +) + +type TaskSkipStatus struct { + IsSkipped bool + SkippingReason SkippingReason +} + // TaskNotFoundError indicates that the resolution failed because a referenced Task couldn't be retrieved type TaskNotFoundError struct { Name string @@ -76,7 +94,7 @@ type ResolvedPipelineRunTask struct { // IsDone returns true only if the task is skipped, succeeded or failed func (t ResolvedPipelineRunTask) IsDone(facts *PipelineRunFacts) bool { - return t.Skip(facts) || t.IsSuccessful() || t.IsFailure() + return t.Skip(facts).IsSkipped || t.IsSuccessful() || t.IsFailure() } // IsRunning returns true only if the task is neither succeeded, cancelled nor failed @@ -172,17 +190,34 @@ func (t *ResolvedPipelineRunTask) checkParentsDone(facts *PipelineRunFacts) bool return true } -func (t *ResolvedPipelineRunTask) skip(facts *PipelineRunFacts) bool { - if facts.isFinalTask(t.PipelineTask.Name) || t.IsStarted() { - return false - } +func (t *ResolvedPipelineRunTask) skip(facts *PipelineRunFacts) TaskSkipStatus { + var skippingReason SkippingReason - if t.conditionsSkip() || t.whenExpressionsSkip(facts) || t.parentTasksSkip(facts) || - facts.IsStopping() || facts.IsGracefullyCancelled() || facts.IsGracefullyStopped() { - return true + switch { + case facts.isFinalTask(t.PipelineTask.Name) || t.IsStarted(): + skippingReason = None + case facts.IsStopping(): + skippingReason = IsStoppingSkip + case facts.IsGracefullyCancelled(): + skippingReason = IsGracefullyCancelledSkip + case facts.IsGracefullyStopped(): + skippingReason = IsGracefullyStoppedSkip + case t.skipBecauseParentTaskWasSkipped(facts): + skippingReason = ParentTasksSkip + case t.skipBecauseConditionsFailed(): + skippingReason = ConditionsSkip + case t.skipBecauseResultReferencesAreMissing(facts): + skippingReason = MissingResultsSkip + case t.skipBecauseWhenExpressionsEvaluatedToFalse(facts): + skippingReason = WhenExpressionsSkip + default: + skippingReason = None } - return false + return TaskSkipStatus{ + IsSkipped: skippingReason != None, + SkippingReason: skippingReason, + } } // Skip returns true if a PipelineTask will not be run because @@ -190,10 +225,11 @@ func (t *ResolvedPipelineRunTask) skip(facts *PipelineRunFacts) bool { // (2) its Condition Checks failed // (3) its parent task was skipped // (4) Pipeline is in stopping state (one of the PipelineTasks failed) +// (5) Pipeline is gracefully cancelled or stopped // Note that this means Skip returns false if a conditionCheck is in progress -func (t *ResolvedPipelineRunTask) Skip(facts *PipelineRunFacts) bool { +func (t *ResolvedPipelineRunTask) Skip(facts *PipelineRunFacts) TaskSkipStatus { if facts.SkipCache == nil { - facts.SkipCache = make(map[string]bool) + facts.SkipCache = make(map[string]TaskSkipStatus) } if _, cached := facts.SkipCache[t.PipelineTask.Name]; !cached { facts.SkipCache[t.PipelineTask.Name] = t.skip(facts) // t.skip() is same as our existing t.Skip() @@ -201,7 +237,9 @@ func (t *ResolvedPipelineRunTask) Skip(facts *PipelineRunFacts) bool { return facts.SkipCache[t.PipelineTask.Name] } -func (t *ResolvedPipelineRunTask) conditionsSkip() bool { +// skipBecauseConditionsFailed checks that the task has Conditions which have completed evaluating +// it returns true if any of the Conditions fails +func (t *ResolvedPipelineRunTask) skipBecauseConditionsFailed() bool { if len(t.ResolvedConditionChecks) > 0 { if t.ResolvedConditionChecks.IsDone() && !t.ResolvedConditionChecks.IsSuccess() { return true @@ -210,26 +248,50 @@ func (t *ResolvedPipelineRunTask) conditionsSkip() bool { return false } -func (t *ResolvedPipelineRunTask) whenExpressionsSkip(facts *PipelineRunFacts) bool { +// skipBecauseWhenExpressionsEvaluatedToFalse confirms that the when expressions have completed evaluating, and +// it returns true if any of the when expressions evaluate to false +func (t *ResolvedPipelineRunTask) skipBecauseWhenExpressionsEvaluatedToFalse(facts *PipelineRunFacts) bool { if t.checkParentsDone(facts) { - if len(t.PipelineTask.WhenExpressions) > 0 { - if !t.PipelineTask.WhenExpressions.HaveVariables() { - if !t.PipelineTask.WhenExpressions.AllowsExecution() { - return true - } - } + if !t.PipelineTask.WhenExpressions.AllowsExecution() { + return true } } return false } -func (t *ResolvedPipelineRunTask) parentTasksSkip(facts *PipelineRunFacts) bool { +// skipBecauseParentTaskWasSkipped loops through the parent tasks and checks if the parent task skipped: +// if yes, is it because of when expressions and are when expressions? +// if yes, it ignores this parent skip and continue evaluating other parent tasks +// if no, it returns true to skip the current task because this parent task was skipped +// if no, it continues checking the other parent tasks +func (t *ResolvedPipelineRunTask) skipBecauseParentTaskWasSkipped(facts *PipelineRunFacts) bool { stateMap := facts.State.ToMap() node := facts.TasksGraph.Nodes[t.PipelineTask.Name] for _, p := range node.Prev { - if stateMap[p.Task.HashKey()].Skip(facts) { + parentTask := stateMap[p.Task.HashKey()] + if parentSkipStatus := parentTask.Skip(facts); parentSkipStatus.IsSkipped { + // if the `when` expressions are scoped to task and the parent task was skipped due to its `when` expressions, + // then we should ignore that and continue evaluating if we should skip because of other parent tasks + if parentSkipStatus.SkippingReason == WhenExpressionsSkip && facts.ScopeWhenExpressionsToTask { + continue + } + return true + } + } + return false +} + +// skipBecauseResultReferencesAreMissing checks if the task references results that cannot be resolved, which is a +// reason for skipping the task, and applies result references if found +func (t *ResolvedPipelineRunTask) skipBecauseResultReferencesAreMissing(facts *PipelineRunFacts) bool { + if t.checkParentsDone(facts) && t.hasResultReferences() { + resolvedResultRefs, pt, err := ResolveResultRefs(facts.State, PipelineRunState{t}) + rprt := facts.State.ToMap()[pt] + if err != nil && (t.IsFinalTask(facts) || rprt.Skip(facts).SkippingReason == WhenExpressionsSkip) { return true } + ApplyTaskResults(PipelineRunState{t}, resolvedResultRefs) + facts.ResetSkippedCache() } return false } @@ -240,19 +302,30 @@ func (t *ResolvedPipelineRunTask) IsFinalTask(facts *PipelineRunFacts) bool { } // IsFinallySkipped returns true if a finally task is not executed and skipped due to task result validation failure -func (t *ResolvedPipelineRunTask) IsFinallySkipped(facts *PipelineRunFacts) bool { - if t.IsStarted() { - return false - } - if facts.checkDAGTasksDone() && facts.isFinalTask(t.PipelineTask.Name) { - if _, err := ResolveResultRef(facts.State, t); err != nil { - return true - } - if t.whenExpressionsSkip(facts) { - return true +func (t *ResolvedPipelineRunTask) IsFinallySkipped(facts *PipelineRunFacts) TaskSkipStatus { + var skippingReason SkippingReason + + switch { + case t.IsStarted(): + skippingReason = None + case facts.checkDAGTasksDone() && facts.isFinalTask(t.PipelineTask.Name): + switch { + case t.skipBecauseResultReferencesAreMissing(facts): + skippingReason = MissingResultsSkip + case t.skipBecauseWhenExpressionsEvaluatedToFalse(facts): + skippingReason = WhenExpressionsSkip + default: + skippingReason = None } + default: + skippingReason = None } - return false + + return TaskSkipStatus{ + IsSkipped: skippingReason != None, + SkippingReason: skippingReason, + } + } // GetRun is a function that will retrieve a Run by name. @@ -584,3 +657,21 @@ func resolvePipelineTaskResources(pt v1beta1.PipelineTask, ts *v1beta1.TaskSpec, } return &rtr, nil } + +func (t *ResolvedPipelineRunTask) hasResultReferences() bool { + for _, param := range t.PipelineTask.Params { + if ps, ok := v1beta1.GetVarSubstitutionExpressionsForParam(param); ok { + if v1beta1.LooksLikeContainsResultRefs(ps) { + return true + } + } + } + for _, we := range t.PipelineTask.WhenExpressions { + if ps, ok := we.GetVarSubstitutionExpressions(); ok { + if v1beta1.LooksLikeContainsResultRefs(ps) { + return true + } + } + } + return false +} diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index 7433e45454f..f59fdce8884 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -190,7 +190,9 @@ var conditionChecks = []v1beta1.TaskRun{{ Namespace: "namespace", Name: "always-true", }, - Spec: v1beta1.TaskRunSpec{}, + Spec: v1beta1.TaskRunSpec{ + Params: []v1beta1.Param{}, + }, }} func makeStarted(tr v1beta1.TaskRun) *v1beta1.TaskRun { @@ -673,9 +675,10 @@ func dagFromState(state PipelineRunState) (*dag.Graph, error) { func TestIsSkipped(t *testing.T) { for _, tc := range []struct { - name string - state PipelineRunState - expected map[string]bool + name string + state PipelineRunState + scopeWhenExpressionsToTask bool + expected map[string]bool }{{ name: "tasks-condition-passed", state: PipelineRunState{{ @@ -983,6 +986,605 @@ func TestIsSkipped(t *testing.T) { expected: map[string]bool{ "mytask13": false, }, + }, { + name: "tasks-with-when-expression-scoped-to-branch", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because parent was skipped and when expressions are scoped to branch + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": true, + "mytask18": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": true, + "mytask18": false, + }, + }, { + name: "tasks-when-expressions-scoped-to-branch-skip-multiple-dependent-tasks", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because parent was skipped and when expressions are scoped to branch + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because parent was skipped and when expressions are scoped to branch + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": true, + "mytask18": true, + "mytask19": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task-run-multiple-dependent-tasks", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its grandparent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": true, + "mytask18": false, + "mytask19": false, + }, + }, { + name: "tasks-when-expressions-scoped-to-task-run-multiple-ordering-and-resource-dependent-tasks", + state: PipelineRunState{{ + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task mytask11 being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its grandparent task mytask11 being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // attempted but skipped because of missing result in params from parent task mytask11 which was skipped + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask20", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + Params: []v1beta1.Param{{ + Name: "commit", + Value: *v1beta1.NewArrayOrString("$(tasks.mytask11.results.missingResult)"), + }}, + }, + TaskRunName: "pipelinerun-resource-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task mytask20 was skipped because of missing result from grandparent task + // mytask11 which was skipped + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask21", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask20"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-3", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // attempted but skipped because of missing result from parent task mytask11 which was skipped in when expressions + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask22", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + WhenExpressions: v1beta1.WhenExpressions{{ + Input: "$(tasks.mytask11.results.missingResult)", + Operator: selection.In, + Values: []string{"expectedResult"}, + }}, + }, + TaskRunName: "pipelinerun-resource-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task mytask22 was skipping because of missing result from grandparent task + // mytask11 which was skipped + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask23", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask22"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-4", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": true, + "mytask18": false, + "mytask19": false, + "mytask20": true, + "mytask21": true, + "mytask22": true, + "mytask23": true, + }, + }, { + name: "tasks-parent-condition-failed-parent-when-expressions-passed-scoped-to-task", + state: PipelineRunState{{ + // skipped because conditions fail + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }, { + // skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task guarded using conditions is skipped, regardless of another parent task + // being guarded with when expressions that are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask6", "mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // not skipped regardless of its parent task being skipped because when expressions are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask6": true, + "mytask11": true, + "mytask18": true, + "mytask19": false, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + d, err := dagFromState(tc.state) + if err != nil { + t.Fatalf("Could not get a dag from the TC state %#v: %v", tc.state, err) + } + stateMap := tc.state.ToMap() + facts := PipelineRunFacts{ + State: tc.state, + TasksGraph: d, + FinalTasksGraph: &dag.Graph{}, + ScopeWhenExpressionsToTask: tc.scopeWhenExpressionsToTask, + } + for taskName, isSkipped := range tc.expected { + rprt := stateMap[taskName] + if rprt == nil { + t.Fatalf("Could not get task %s from the state: %v", taskName, tc.state) + } + if d := cmp.Diff(isSkipped, rprt.Skip(&facts).IsSkipped); d != "" { + t.Errorf("Didn't get expected isSkipped from task %s: %s", taskName, diff.PrintWantGot(d)) + } + } + }) + } +} + +func TestSkipBecauseParentTaskWasSkipped(t *testing.T) { + for _, tc := range []struct { + name string + state PipelineRunState + scopeWhenExpressionsToTask bool + expected map[string]bool + }{{ + name: "tasks-parent-condition-passed", + state: PipelineRunState{{ + // parent task that has a condition that passed + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: successTaskConditionCheckState, + }, { + // child task that's not skipped (conditional parent task was not skipped) + PipelineTask: &pts[6], + }}, + expected: map[string]bool{ + "mytask7": false, + }, + }, { + name: "tasks-parent-condition-failed", + state: PipelineRunState{{ + // parent task that has a condition that failed and is skipped + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }, { + // child task skipped because its parent has a condition that failed (and was skipped) + PipelineTask: &pts[6], + }}, + expected: map[string]bool{ + "mytask7": true, + }, + }, { + name: "tasks-parent-condition-running", + state: PipelineRunState{{ + // parent task has a condition that's still running + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: TaskConditionCheckState{{ + ConditionCheckName: "myconditionCheck", + Condition: &condition, + ConditionCheck: v1beta1.NewConditionCheck(makeStarted(conditionChecks[0])), + }}, + }, { + // child task not skipped because parent is not yet done + PipelineTask: &pts[6], + }}, + expected: map[string]bool{ + "mytask7": false, + }, + }, { + name: "when-expression-task-but-without-parent-done", + state: PipelineRunState{{ + // parent task has when expressions but is not yet done + PipelineTask: &pts[0], + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task not skipped because parent is not yet done + PipelineTask: &pts[11], + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + expected: map[string]bool{ + "mytask12": false, + }, + }, { + name: "tasks-with-when-expression-scoped-to-branch", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is skipped because parent was skipped due to its when expressions evaluating to false when + // they are scoped to task and its dependent tasks + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": false, + "mytask18": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": false, + "mytask18": false, + }, + }, { + name: "tasks-when-expressions-scoped-to-branch-skip-multiple-dependent-tasks", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is skipped because parent was skipped due to its when expressions evaluating to false when + // they are scoped to task and its dependent tasks + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is skipped because parent was skipped due to its when expressions evaluating to false when + // they are scoped to task and its dependent tasks + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: false, + expected: map[string]bool{ + "mytask11": false, + "mytask18": true, + "mytask19": true, + }, + }, { + name: "tasks-when-expressions-scoped-to-task-run-multiple-dependent-tasks", + state: PipelineRunState{{ + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask18"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask11": false, + "mytask18": false, + "mytask19": false, + }, + }, { + name: "tasks-parent-condition-failed-parent-when-expressions-passed-scoped-to-task", + state: PipelineRunState{{ + // parent task is skipped because conditions fail, not because of its parent tasks + PipelineTask: &pts[5], + TaskRunName: "pipelinerun-conditionaltask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + ResolvedConditionChecks: failedTaskConditionCheckState, + }, { + // parent task is skipped because when expressions evaluate to false, not because of its parent tasks + PipelineTask: &pts[10], + TaskRunName: "pipelinerun-guardedtask", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // skipped because of parent task guarded using conditions is skipped, regardless of another parent task + // being guarded with when expressions that are scoped to task + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask18", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask6", "mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-1", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }, { + // child task is not skipped regardless of its parent task being skipped due to when expressions evaluating + // to false, because when expressions are scoped to task only + PipelineTask: &v1beta1.PipelineTask{ + Name: "mytask19", + TaskRef: &v1beta1.TaskRef{Name: "task"}, + RunAfter: []string{"mytask11"}, + }, + TaskRunName: "pipelinerun-ordering-dependent-task-2", + TaskRun: nil, + ResolvedTaskResources: &resources.ResolvedTaskResources{ + TaskSpec: &task.Spec, + }, + }}, + scopeWhenExpressionsToTask: true, + expected: map[string]bool{ + "mytask6": false, + "mytask11": false, + "mytask18": true, + "mytask19": false, + }, }} { t.Run(tc.name, func(t *testing.T) { d, err := dagFromState(tc.state) @@ -991,17 +1593,18 @@ func TestIsSkipped(t *testing.T) { } stateMap := tc.state.ToMap() facts := PipelineRunFacts{ - State: tc.state, - TasksGraph: d, - FinalTasksGraph: &dag.Graph{}, + State: tc.state, + TasksGraph: d, + FinalTasksGraph: &dag.Graph{}, + ScopeWhenExpressionsToTask: tc.scopeWhenExpressionsToTask, } for taskName, isSkipped := range tc.expected { rprt := stateMap[taskName] if rprt == nil { t.Fatalf("Could not get task %s from the state: %v", taskName, tc.state) } - if d := cmp.Diff(isSkipped, rprt.Skip(&facts)); d != "" { - t.Errorf("Didn't get expected isSkipped %s", diff.PrintWantGot(d)) + if d := cmp.Diff(isSkipped, rprt.skipBecauseParentTaskWasSkipped(&facts)); d != "" { + t.Errorf("Didn't get expected isSkipped from task %s: %s", taskName, diff.PrintWantGot(d)) } } }) @@ -2408,7 +3011,7 @@ func TestResolvedPipelineRunTask_IsFinallySkipped(t *testing.T) { for i := range state { if i > 0 { // first one is a dag task that produces a result finallyTaskName := state[i].PipelineTask.Name - if d := cmp.Diff(expected[finallyTaskName], state[i].IsFinallySkipped(facts)); d != "" { + if d := cmp.Diff(expected[finallyTaskName], state[i].IsFinallySkipped(facts).IsSkipped); d != "" { t.Fatalf("Didn't get expected isFinallySkipped from finally task %s: %s", finallyTaskName, diff.PrintWantGot(d)) } } diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go index 1feea87443e..078b52fe262 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go @@ -41,7 +41,10 @@ const ( // state of the PipelineRun. type PipelineRunState []*ResolvedPipelineRunTask -// PipelineRunFacts is a collection of list of ResolvedPipelineTask, graph of DAG tasks, and graph of finally tasks +// PipelineRunFacts holds the state of all the components that make up the Pipeline graph that are used to track the +// PipelineRun state without passing all these components separately. It helps simplify our implementation for getting +// and scheduling the next tasks. It is a collection of list of ResolvedPipelineTask, graph of DAG tasks, graph of +// finally tasks, cache of skipped tasks, and the scope of when expressions. type PipelineRunFacts struct { State PipelineRunState SpecStatus v1beta1.PipelineRunSpecStatus @@ -56,7 +59,8 @@ type PipelineRunFacts struct { // needed, via the `Skip` method in pipelinerunresolution.go // The skip data is sensitive to changes in the state. The ResetSkippedCache method // can be used to clean the cache and force re-computation when needed. - SkipCache map[string]bool + SkipCache map[string]TaskSkipStatus + ScopeWhenExpressionsToTask bool } // pipelineRunStatusCount holds the count of successful, failed, cancelled, skipped, and incomplete tasks @@ -75,7 +79,7 @@ type pipelineRunStatusCount struct { // ResetSkippedCache resets the skipped cache in the facts map func (facts *PipelineRunFacts) ResetSkippedCache() { - facts.SkipCache = make(map[string]bool) + facts.SkipCache = make(map[string]TaskSkipStatus) } // ToMap returns a map that maps pipeline task name to the resolved pipeline run task @@ -406,14 +410,14 @@ func (facts *PipelineRunFacts) GetPipelineConditionStatus(pr *v1beta1.PipelineRu func (facts *PipelineRunFacts) GetSkippedTasks() []v1beta1.SkippedTask { var skipped []v1beta1.SkippedTask for _, rprt := range facts.State { - if rprt.Skip(facts) { + if rprt.Skip(facts).IsSkipped { skippedTask := v1beta1.SkippedTask{ Name: rprt.PipelineTask.Name, WhenExpressions: rprt.PipelineTask.WhenExpressions, } skipped = append(skipped, skippedTask) } - if rprt.IsFinallySkipped(facts) { + if rprt.IsFinallySkipped(facts).IsSkipped { skippedTask := v1beta1.SkippedTask{ Name: rprt.PipelineTask.Name, } @@ -465,7 +469,7 @@ func (facts *PipelineRunFacts) GetPipelineTaskStatus() map[string]string { } // if any of the dag task skipped, change the aggregate status to completed // but continue checking for any other failure - if t.Skip(facts) { + if t.Skip(facts).IsSkipped { aggregateStatus = v1beta1.PipelineRunReasonCompleted.String() } } @@ -481,7 +485,7 @@ func (facts *PipelineRunFacts) successfulOrSkippedDAGTasks() []string { tasks := []string{} for _, t := range facts.State { if facts.isDAGTask(t.PipelineTask.Name) { - if t.IsSuccessful() || t.Skip(facts) { + if t.IsSuccessful() || t.Skip(facts).IsSkipped { tasks = append(tasks, t.PipelineTask.Name) } } @@ -533,10 +537,10 @@ func (facts *PipelineRunFacts) getPipelineTasksCount() pipelineRunStatusCount { case t.IsFailure(): s.Failed++ // increment skip counter since the task is skipped - case t.Skip(facts): + case t.Skip(facts).IsSkipped: s.Skipped++ // checking if any finally tasks were referring to invalid/missing task results - case t.IsFinallySkipped(facts): + case t.IsFinallySkipped(facts).IsSkipped: s.Skipped++ // increment incomplete counter since the task is pending and not executed yet default: diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go index 4cd86c978ea..bc2be0090f0 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution.go @@ -38,25 +38,25 @@ type ResolvedResultRef struct { } // ResolveResultRef resolves any ResultReference that are found in the target ResolvedPipelineRunTask -func ResolveResultRef(pipelineRunState PipelineRunState, target *ResolvedPipelineRunTask) (ResolvedResultRefs, error) { - resolvedResultRefs, err := convertToResultRefs(pipelineRunState, target) +func ResolveResultRef(pipelineRunState PipelineRunState, target *ResolvedPipelineRunTask) (ResolvedResultRefs, string, error) { + resolvedResultRefs, pt, err := convertToResultRefs(pipelineRunState, target) if err != nil { - return nil, err + return nil, pt, err } - return resolvedResultRefs, nil + return removeDup(resolvedResultRefs), "", nil } // ResolveResultRefs resolves any ResultReference that are found in the target ResolvedPipelineRunTask -func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunState) (ResolvedResultRefs, error) { +func ResolveResultRefs(pipelineRunState PipelineRunState, targets PipelineRunState) (ResolvedResultRefs, string, error) { var allResolvedResultRefs ResolvedResultRefs for _, target := range targets { - resolvedResultRefs, err := convertToResultRefs(pipelineRunState, target) + resolvedResultRefs, pt, err := convertToResultRefs(pipelineRunState, target) if err != nil { - return nil, err + return nil, pt, err } allResolvedResultRefs = append(allResolvedResultRefs, resolvedResultRefs...) } - return removeDup(allResolvedResultRefs), nil + return removeDup(allResolvedResultRefs), "", nil } // extractResultRefs resolves any ResultReference that are found in param or pipeline result @@ -73,7 +73,7 @@ func extractResultRefs(expressions []string, pipelineRunState PipelineRunState) resultRefs := v1beta1.NewResultRefs(expressions) var resolvedResultRefs ResolvedResultRefs for _, resultRef := range resultRefs { - resolvedResultRef, err := resolveResultRef(pipelineRunState, resultRef) + resolvedResultRef, _, err := resolveResultRef(pipelineRunState, resultRef) if err != nil { return nil, err } @@ -117,25 +117,25 @@ func removeDup(refs ResolvedResultRefs) ResolvedResultRefs { // found they are resolved to a value by searching pipelineRunState. The list of resolved // references are returned. If an error is encountered due to an invalid result reference // then a nil list and error is returned instead. -func convertToResultRefs(pipelineRunState PipelineRunState, target *ResolvedPipelineRunTask) (ResolvedResultRefs, error) { +func convertToResultRefs(pipelineRunState PipelineRunState, target *ResolvedPipelineRunTask) (ResolvedResultRefs, string, error) { var resolvedResultRefs ResolvedResultRefs for _, ref := range v1beta1.PipelineTaskResultRefs(target.PipelineTask) { - resolved, err := resolveResultRef(pipelineRunState, ref) + resolved, pt, err := resolveResultRef(pipelineRunState, ref) if err != nil { - return nil, err + return nil, pt, err } resolvedResultRefs = append(resolvedResultRefs, resolved) } - return resolvedResultRefs, nil + return resolvedResultRefs, "", nil } -func resolveResultRef(pipelineState PipelineRunState, resultRef *v1beta1.ResultRef) (*ResolvedResultRef, error) { +func resolveResultRef(pipelineState PipelineRunState, resultRef *v1beta1.ResultRef) (*ResolvedResultRef, string, error) { referencedPipelineTask := pipelineState.ToMap()[resultRef.PipelineTask] if referencedPipelineTask == nil { - return nil, fmt.Errorf("could not find task %q referenced by result", resultRef.PipelineTask) + return nil, resultRef.PipelineTask, fmt.Errorf("could not find task %q referenced by result", resultRef.PipelineTask) } if !referencedPipelineTask.IsSuccessful() { - return nil, fmt.Errorf("task %q referenced by result was not successful", referencedPipelineTask.PipelineTask.Name) + return nil, resultRef.PipelineTask, fmt.Errorf("task %q referenced by result was not successful", referencedPipelineTask.PipelineTask.Name) } var runName, taskRunName, resultValue string @@ -144,13 +144,13 @@ func resolveResultRef(pipelineState PipelineRunState, resultRef *v1beta1.ResultR runName = referencedPipelineTask.Run.Name resultValue, err = findRunResultForParam(referencedPipelineTask.Run, resultRef) if err != nil { - return nil, err + return nil, resultRef.PipelineTask, err } } else { taskRunName = referencedPipelineTask.TaskRun.Name resultValue, err = findTaskResultForParam(referencedPipelineTask.TaskRun, resultRef) if err != nil { - return nil, err + return nil, resultRef.PipelineTask, err } } @@ -159,7 +159,7 @@ func resolveResultRef(pipelineState PipelineRunState, resultRef *v1beta1.ResultR FromTaskRun: taskRunName, FromRun: runName, ResultReference: *resultRef, - }, nil + }, "", nil } func findRunResultForParam(run *v1alpha1.Run, reference *v1beta1.ResultRef) (string, error) { diff --git a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go index 28ffbed5ee3..5a9af8e2aff 100644 --- a/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go @@ -29,6 +29,86 @@ var ( } ) +var pipelineRunState = PipelineRunState{{ + TaskRunName: "aTaskRun", + TaskRun: tb.TaskRun("aTaskRun", tb.TaskRunStatus( + tb.StatusCondition(successCondition), + tb.TaskRunResult("aResult", "aResultValue"), + )), + PipelineTask: &v1beta1.PipelineTask{ + Name: "aTask", + TaskRef: &v1beta1.TaskRef{Name: "aTask"}, + }, +}, { + PipelineTask: &v1beta1.PipelineTask{ + Name: "bTask", + TaskRef: &v1beta1.TaskRef{Name: "bTask"}, + Params: []v1beta1.Param{{ + Name: "bParam", + Value: *v1beta1.NewArrayOrString("$(tasks.aTask.results.aResult)"), + }}, + }, +}, { + PipelineTask: &v1beta1.PipelineTask{ + Name: "bTask", + TaskRef: &v1beta1.TaskRef{Name: "bTask"}, + WhenExpressions: []v1beta1.WhenExpression{{ + Input: "$(tasks.aTask.results.aResult)", + Operator: selection.In, + Values: []string{"$(tasks.aTask.results.aResult)"}, + }}, + }, +}, { + PipelineTask: &v1beta1.PipelineTask{ + Name: "bTask", + TaskRef: &v1beta1.TaskRef{Name: "bTask"}, + WhenExpressions: []v1beta1.WhenExpression{{ + Input: "$(tasks.aTask.results.missingResult)", + Operator: selection.In, + Values: []string{"$(tasks.aTask.results.missingResult)"}, + }}, + }, +}, { + PipelineTask: &v1beta1.PipelineTask{ + Name: "bTask", + TaskRef: &v1beta1.TaskRef{Name: "bTask"}, + Params: []v1beta1.Param{{ + Name: "bParam", + Value: *v1beta1.NewArrayOrString("$(tasks.aTask.results.missingResult)"), + }}, + }, +}, { + CustomTask: true, + RunName: "aRun", + Run: &v1alpha1.Run{ + ObjectMeta: metav1.ObjectMeta{Name: "aRun"}, + Status: v1alpha1.RunStatus{ + Status: duckv1.Status{ + Conditions: []apis.Condition{successCondition}, + }, + RunStatusFields: v1alpha1.RunStatusFields{ + Results: []v1alpha1.RunResult{{ + Name: "aResult", + Value: "aResultValue", + }}, + }, + }, + }, + PipelineTask: &v1beta1.PipelineTask{ + Name: "aCustomPipelineTask", + TaskRef: &v1beta1.TaskRef{APIVersion: "example.dev/v0", Kind: "Example", Name: "aTask"}, + }, +}, { + PipelineTask: &v1beta1.PipelineTask{ + Name: "bTask", + TaskRef: &v1beta1.TaskRef{Name: "bTask"}, + Params: []v1beta1.Param{{ + Name: "bParam", + Value: *v1beta1.NewArrayOrString("$(tasks.aCustomPipelineTask.results.aResult)"), + }}, + }, +}} + func TestTaskParamResolver_ResolveResultRefs(t *testing.T) { for _, tt := range []struct { @@ -314,92 +394,13 @@ func resolvedSliceAsString(rs []*ResolvedResultRef) string { } func TestResolveResultRefs(t *testing.T) { - pipelineRunState := PipelineRunState{{ - TaskRunName: "aTaskRun", - TaskRun: tb.TaskRun("aTaskRun", tb.TaskRunStatus( - tb.StatusCondition(successCondition), - tb.TaskRunResult("aResult", "aResultValue"), - )), - PipelineTask: &v1beta1.PipelineTask{ - Name: "aTask", - TaskRef: &v1beta1.TaskRef{Name: "aTask"}, - }, - }, { - PipelineTask: &v1beta1.PipelineTask{ - Name: "bTask", - TaskRef: &v1beta1.TaskRef{Name: "bTask"}, - Params: []v1beta1.Param{{ - Name: "bParam", - Value: *v1beta1.NewArrayOrString("$(tasks.aTask.results.aResult)"), - }}, - }, - }, { - PipelineTask: &v1beta1.PipelineTask{ - Name: "bTask", - TaskRef: &v1beta1.TaskRef{Name: "bTask"}, - WhenExpressions: []v1beta1.WhenExpression{{ - Input: "$(tasks.aTask.results.aResult)", - Operator: selection.In, - Values: []string{"$(tasks.aTask.results.aResult)"}, - }}, - }, - }, { - PipelineTask: &v1beta1.PipelineTask{ - Name: "bTask", - TaskRef: &v1beta1.TaskRef{Name: "bTask"}, - WhenExpressions: []v1beta1.WhenExpression{{ - Input: "$(tasks.aTask.results.missingResult)", - Operator: selection.In, - Values: []string{"$(tasks.aTask.results.missingResult)"}, - }}, - }, - }, { - PipelineTask: &v1beta1.PipelineTask{ - Name: "bTask", - TaskRef: &v1beta1.TaskRef{Name: "bTask"}, - Params: []v1beta1.Param{{ - Name: "bParam", - Value: *v1beta1.NewArrayOrString("$(tasks.aTask.results.missingResult)"), - }}, - }, - }, { - CustomTask: true, - RunName: "aRun", - Run: &v1alpha1.Run{ - ObjectMeta: metav1.ObjectMeta{Name: "aRun"}, - Status: v1alpha1.RunStatus{ - Status: duckv1.Status{ - Conditions: []apis.Condition{successCondition}, - }, - RunStatusFields: v1alpha1.RunStatusFields{ - Results: []v1alpha1.RunResult{{ - Name: "aResult", - Value: "aResultValue", - }}, - }, - }, - }, - PipelineTask: &v1beta1.PipelineTask{ - Name: "aCustomPipelineTask", - TaskRef: &v1beta1.TaskRef{APIVersion: "example.dev/v0", Kind: "Example", Name: "aTask"}, - }, - }, { - PipelineTask: &v1beta1.PipelineTask{ - Name: "bTask", - TaskRef: &v1beta1.TaskRef{Name: "bTask"}, - Params: []v1beta1.Param{{ - Name: "bParam", - Value: *v1beta1.NewArrayOrString("$(tasks.aCustomPipelineTask.results.aResult)"), - }}, - }, - }} - for _, tt := range []struct { name string pipelineRunState PipelineRunState targets PipelineRunState want ResolvedResultRefs wantErr bool + wantPt string }{{ name: "Test successful result references resolution - params", pipelineRunState: pipelineRunState, @@ -446,6 +447,7 @@ func TestResolveResultRefs(t *testing.T) { }, want: nil, wantErr: true, + wantPt: "aTask", }, { name: "Test unsuccessful result references resolution - params", pipelineRunState: pipelineRunState, @@ -454,6 +456,7 @@ func TestResolveResultRefs(t *testing.T) { }, want: nil, wantErr: true, + wantPt: "aTask", }, { name: "Test successful result references resolution - params - Run", pipelineRunState: pipelineRunState, @@ -471,7 +474,7 @@ func TestResolveResultRefs(t *testing.T) { wantErr: false, }} { t.Run(tt.name, func(t *testing.T) { - got, err := ResolveResultRefs(tt.pipelineRunState, tt.targets) + got, pt, err := ResolveResultRefs(tt.pipelineRunState, tt.targets) sort.SliceStable(got, func(i, j int) bool { fromI := got[i].FromTaskRun if fromI == "" { @@ -490,6 +493,104 @@ func TestResolveResultRefs(t *testing.T) { if d := cmp.Diff(tt.want, got); d != "" { t.Fatalf("ResolveResultRef %s", diff.PrintWantGot(d)) } + if d := cmp.Diff(tt.wantPt, pt); d != "" { + t.Fatalf("ResolvedPipelineRunTask %s", diff.PrintWantGot(d)) + } + }) + } +} + +func TestResolveResultRef(t *testing.T) { + for _, tt := range []struct { + name string + pipelineRunState PipelineRunState + target *ResolvedPipelineRunTask + want ResolvedResultRefs + wantErr bool + wantPt string + }{{ + name: "Test successful result references resolution - params", + pipelineRunState: pipelineRunState, + target: pipelineRunState[1], + want: ResolvedResultRefs{{ + Value: *v1beta1.NewArrayOrString("aResultValue"), + ResultReference: v1beta1.ResultRef{ + PipelineTask: "aTask", + Result: "aResult", + }, + FromTaskRun: "aTaskRun", + }}, + wantErr: false, + }, { + name: "Test successful result references resolution - when expressions", + pipelineRunState: pipelineRunState, + target: pipelineRunState[2], + want: ResolvedResultRefs{{ + Value: *v1beta1.NewArrayOrString("aResultValue"), + ResultReference: v1beta1.ResultRef{ + PipelineTask: "aTask", + Result: "aResult", + }, + FromTaskRun: "aTaskRun", + }}, + wantErr: false, + }, { + name: "Test successful result references resolution non result references", + pipelineRunState: pipelineRunState, + target: pipelineRunState[0], + want: nil, + wantErr: false, + }, { + name: "Test unsuccessful result references resolution - when expression", + pipelineRunState: pipelineRunState, + target: pipelineRunState[3], + want: nil, + wantErr: true, + wantPt: "aTask", + }, { + name: "Test unsuccessful result references resolution - params", + pipelineRunState: pipelineRunState, + target: pipelineRunState[4], + want: nil, + wantErr: true, + wantPt: "aTask", + }, { + name: "Test successful result references resolution - params - Run", + pipelineRunState: pipelineRunState, + target: pipelineRunState[6], + want: ResolvedResultRefs{{ + Value: *v1beta1.NewArrayOrString("aResultValue"), + ResultReference: v1beta1.ResultRef{ + PipelineTask: "aCustomPipelineTask", + Result: "aResult", + }, + FromRun: "aRun", + }}, + wantErr: false, + }} { + t.Run(tt.name, func(t *testing.T) { + got, pt, err := ResolveResultRef(tt.pipelineRunState, tt.target) + sort.SliceStable(got, func(i, j int) bool { + fromI := got[i].FromTaskRun + if fromI == "" { + fromI = got[i].FromRun + } + fromJ := got[j].FromTaskRun + if fromJ == "" { + fromJ = got[j].FromRun + } + return strings.Compare(fromI, fromJ) < 0 + }) + if (err != nil) != tt.wantErr { + t.Errorf("ResolveResultRefs() error = %v, wantErr %v", err, tt.wantErr) + return + } + if d := cmp.Diff(tt.want, got); d != "" { + t.Fatalf("ResolveResultRef %s", diff.PrintWantGot(d)) + } + if d := cmp.Diff(tt.wantPt, pt); d != "" { + t.Fatalf("ResolvedPipelineRunTask %s", diff.PrintWantGot(d)) + } }) } }