Skip to content

don't validate skipped task results for pipeline results #6157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tekton-robot merged 1 commit into from
Mar 16, 2023
Merged

don't validate skipped task results for pipeline results #6157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions docs/pipelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ tasks:
value: https://github.com/tektoncd/catalog.git
- name: revision
# value can use params declared at the pipeline level or a static value like main
value: $(params.gitRevision)
value: $(params.gitRevision)
- name: pathInRepo
value: task/golang-build/0.3/golang-build.yaml
```
Expand Down Expand Up @@ -1079,6 +1079,7 @@ This will cause an `InvalidTaskResultReference` validation error during `Pipelin

**Note:** Since a `Pipeline Result` can contain references to multiple `Task Results`, if any of those
`Task Result` references are invalid the entire `Pipeline Result` is not emitted.
**Note:** If a `PipelineTask` referenced by the `Pipeline Result` was skipped, the `Pipeline Result` will not be emitted and the `PipelineRun` will not fail due to a missing result.

## Configuring the `Task` execution order

Expand Down Expand Up @@ -1322,7 +1323,7 @@ results:
value: $(finally.check-count.results.comment-count-validate)
finally:
- name: check-count
taskRef:
taskRef:
name: example-task-name
```

Expand Down Expand Up @@ -1810,7 +1811,7 @@ Consult the documentation of the custom task that you are using to determine whe
Pipelines do not support the following items with custom tasks:
* Pipeline Resources

### Known Custom Tasks
### Known Custom Tasks

We try to list as many known Custom Tasks as possible here so that users can easily find what they want. Please feel free to share the Custom Task you implemented in this table.

Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,8 +722,8 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get

pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse {
pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(pipelineSpec.Results,
pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults())
pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results,
pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks)
if err != nil {
pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
return err
Expand Down
15 changes: 14 additions & 1 deletion pkg/reconciler/pipelinerun/resources/apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strings"

"github.com/tektoncd/pipeline/pkg/apis/config"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
"github.com/tektoncd/pipeline/pkg/substitution"
Expand Down Expand Up @@ -313,11 +314,18 @@ func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]st
// and omitted from the returned slice. A nil slice is returned if no results are passed in or all
// results are invalid.
func ApplyTaskResultsToPipelineResults(
ctx context.Context,
results []v1beta1.PipelineResult,
taskRunResults map[string][]v1beta1.TaskRunResult,
customTaskResults map[string][]v1beta1.CustomRunResult) ([]v1beta1.PipelineRunResult, error) {
customTaskResults map[string][]v1beta1.CustomRunResult,
skippedTasks []v1beta1.SkippedTask) ([]v1beta1.PipelineRunResult, error) {
var runResults []v1beta1.PipelineRunResult
var invalidPipelineResults []string
skippedTaskNames := map[string]bool{}
for _, t := range skippedTasks {
skippedTaskNames[t.Name] = true
}

Comment on lines +324 to +328

@jerop jerop Feb 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we please add a log message or something to communicate to users that a pipelineresult references a skipped taskresult so the value cannot be substituted?

@Yongxuanzhang Yongxuanzhang Feb 14, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@Yongxuanzhang Yongxuanzhang Feb 14, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a log message when we skip the skipped task result reference

@afrittoli afrittoli Feb 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The controller log file won't typically be accessible by users, and having a log entry in the "info" level about this feels like something that might spam logs if users often skipped tasks. I would rather have a debug log level here.

If we want to let users know about this, perhaps we could replace the result reference (or extend it) with a comment, saying that the result is not available as the task was skipped, but I'm not sure if that'd be a good idea.

@afrittoli afrittoli Mar 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yongxuanzhang any update on this? I still see an info-level log message on L352

@Yongxuanzhang Yongxuanzhang Mar 10, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry I missed your comment! @afrittoli
I will change this to debug level log.

stringReplacements := map[string]string{}
arrayReplacements := map[string][]string{}
objectReplacements := map[string]map[string]string{}
Expand All @@ -338,6 +346,11 @@ func ApplyTaskResultsToPipelineResults(
continue
}
variableParts := strings.Split(variable, ".")
// if the referenced task is skipped, we should also skip the results replacements
if _, ok := skippedTaskNames[variableParts[1]]; ok {

@afrittoli afrittoli Feb 16, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something to be changed in this PR, but I wonder if having constants for the various indexes of parts and a function for the split would make this code more readable? Like,

variableParts := splitResultVariable(variable)
skippedTaskNames[variableParts[VARIABLE_RESULT_TASK_NAME_INDEX]]

@Yongxuanzhang Yongxuanzhang Feb 16, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I can fix this

validPipelineResult = false
continue
}
if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart {
validPipelineResult = false
invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
Expand Down
24 changes: 22 additions & 2 deletions pkg/reconciler/pipelinerun/resources/apply_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3131,7 +3131,7 @@ func TestApplyFinallyResultsToPipelineResults(t *testing.T) {
},
} {
t.Run(tc.description, func(t *testing.T) {
received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults)
received, _ := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /* skippedTasks */)
if d := cmp.Diff(tc.expected, received); d != "" {
t.Errorf(diff.PrintWantGot(d))
}
Expand All @@ -3145,6 +3145,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) {
results []v1beta1.PipelineResult
taskResults map[string][]v1beta1.TaskRunResult
runResults map[string][]v1beta1.CustomRunResult
skippedTasks []v1beta1.SkippedTask
expectedResults []v1beta1.PipelineRunResult
expectedError error
}{{
Expand Down Expand Up @@ -3599,9 +3600,28 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) {
Name: "pipeline-result-2",
Value: *v1beta1.NewStructuredValues("do, rae, mi, rae, do"),
}},
}, {
description: "multiple-results-skipped-and-normal-tasks",
results: []v1beta1.PipelineResult{{
Name: "pipeline-result-1",
Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo)"),
}, {
Name: "pipeline-result-2",
Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo), $(tasks.normaltask.results.baz)"),
}},
taskResults: map[string][]v1beta1.TaskRunResult{
"normaltask": {{
Name: "baz",
Value: *v1beta1.NewStructuredValues("rae"),
}},
},
skippedTasks: []v1beta1.SkippedTask{{
Name: "skippedTask",
}},
expectedResults: nil,
}} {
t.Run(tc.description, func(t *testing.T) {
received, err := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults)
received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks)
if tc.expectedError != nil {
if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" {
t.Errorf("ApplyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d))
Expand Down