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

Yongxuanzhang · 2023-02-13T19:24:53Z

Changes

This commit skip the validation for skipped task results in pipeline results. This fixes #6139. The skipped task results are not emitted by design thus we shouldn't validate if they are emitted. Pipeline results referenced to skipped task will not be emitted.

/kind bug

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

A fix for regression issue that previously we don't fail the run when skipped task results don't emit, but failed when validation is introduced.

tekton-robot · 2023-02-13T19:24:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tekton-robot · 2023-02-13T19:32:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	97.5%	97.5%	0.1

tekton-robot · 2023-02-13T19:35:45Z

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/resources/apply.go	97.5%	97.5%	0.1

Yongxuanzhang · 2023-02-13T19:44:58Z

Hi @pritidesai @jerop , could you take a look at this PR?

kyubisation

Thank you very much for the quick fix! 😃
I'm not very familiar with the code base, but from my perspective this looks good to me. 👍

pkg/reconciler/pipelinerun/resources/apply.go

jerop · 2023-02-13T20:58:52Z

please fix the typo in the pr title and commit message

jerop

could it be more confusing for the pipeline result to remain as is - an unsubstituted reference to a task result? what if users assume pipeline results were produced by the pipeline, but then some are just the original references? wondering if we should just wait for tektoncd/community#954?

also @Yongxuanzhang please add to the commit message that this change is not only removing validation, but also leaving pipeline results as the original unsubstituted references to task results when the task was skipped

tekton-robot · 2023-02-13T21:10:04Z

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/resources/apply.go	97.5%	97.5%	0.1

tekton-robot · 2023-02-13T21:12:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	97.5%	97.5%	0.1

Yongxuanzhang · 2023-02-13T21:15:32Z

could it be more confusing for the pipeline result to remain as is - an unsubstituted reference to a task result? what if users assume pipeline results were produced by the pipeline, but then some are just the original references? wondering if we should just wait for tektoncd/community#954?

also @Yongxuanzhang please add to the commit message that this change is not only removing validation, but also leaving pipeline results as the original unsubstituted references to task results when the task was skipped

This commit skips the validation of "skipped tasks", my understanding is that if users use "when expressions" on tasks and they should be aware of some results cannot be replaced if tasks are skipped as mentioned in https://tekton.dev/docs/pipelines/pipelines/#emitting-results-from-a-pipeline

tekton-robot · 2023-03-06T16:12:51Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	97.5%	97.5%	0.1

pritidesai · 2023-03-06T17:23:28Z

Please update the release notes elaborating its a fix for a regression issue, thanks!

tekton-robot · 2023-03-10T19:04:04Z

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/resources/apply.go	97.5%	97.5%	0.1

tekton-robot · 2023-03-10T19:06:26Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	97.5%	97.5%	0.1

lbernick

I think it would be worth adding a reconciler unit test or an integration test with a full pipelinerun spec with skipped results

docs/pipelines.md

lbernick · 2023-03-15T23:36:20Z

pkg/reconciler/pipelinerun/resources/apply.go

-	customTaskResults map[string][]v1beta1.CustomRunResult) ([]v1beta1.PipelineRunResult, error) {
+	customTaskResults map[string][]v1beta1.CustomRunResult,
+	skippedTasks []v1beta1.SkippedTask,
+	logger *zap.SugaredLogger) ([]v1beta1.PipelineRunResult, error) {


nit: I think it would be a bit cleaner to get the logger from the context instead of passing it in as a param (more consistent with the rest of the codebase)

Oh I see, thanks! Fixed!

lbernick · 2023-03-15T23:37:11Z

pkg/reconciler/pipelinerun/resources/apply_test.go

 	} {
 		t.Run(tc.description, func(t *testing.T) {
-			received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults)
+			received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, nil, logtesting.TestLogger(t))


Suggested change

received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, nil, logtesting.TestLogger(t))

received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, nil /* skippedTasks */, logtesting.TestLogger(t))

lbernick · 2023-03-15T23:40:52Z

pkg/reconciler/pipelinerun/resources/apply_test.go

+		skippedTasks: []v1beta1.SkippedTask{{
+			Name: "skippedTask",
+		}},
+		expectedResults: []v1beta1.PipelineRunResult{{


What output results existed prior to this regression? Was it just a non-replaced result expression? or was there no pipeline result? (based on the linked issue it looks like the latter.) I think an absent Pipeline result might make more sense than a Pipeline result like "$(tasks.skippedTask.results.foo)".

(everything else is mostly a nit but let's verify this)

Ah yes this is a great point! When I implemented this fix I didn't check the previous behaviour. I checked the previous version and there are no results if the task is skipped. We should definitely keep the previous behaviour otherwise this will be another issue

tekton-robot · 2023-03-16T03:09:51Z

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/resources/apply.go	98.0%	98.1%	0.1

tekton-robot · 2023-03-16T03:15:40Z

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/resources/apply.go	98.0%	98.1%	0.1

tekton-robot · 2023-03-16T03:21:46Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.0%	98.1%	0.1

tekton-robot · 2023-03-16T03:27:36Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.0%	98.1%	0.1

This commit skip the validation for skipped task results in pipeline results. This fixes tektoncd#6139. The skipped task results are not emitted by design thus we shouldn't validate if they are emitted. Pipeline results referenced to skipped task will not be emitted.

tekton-robot · 2023-03-16T04:04:13Z

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/resources/apply.go	98.0%	98.1%	0.1

tekton-robot · 2023-03-16T04:05:47Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.0%	98.1%	0.1

tekton-robot · 2023-03-16T04:31:46Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/apply.go	98.0%	98.1%	0.1

afrittoli

Thank you for all the updates!
/approve

tekton-robot · 2023-03-16T10:05:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, kyubisation

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [afrittoli]

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

lbernick · 2023-03-16T14:05:12Z

/lgtm

I think we'll need to cherry pick this to all the releases after v0.37 we currently support-- which is v0.41-v0.45 :( (Although v0.45 EOL is tomorrow)

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesnt merit a release note. labels Feb 13, 2023

tekton-robot requested review from bobcatfish and lbernick February 13, 2023 19:24

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 13, 2023

Yongxuanzhang mentioned this pull request Feb 13, 2023

Pipeline failing with "invalid pipelineresults" when referencing results of skipped tasks #6139

Closed

Yongxuanzhang marked this pull request as ready for review February 13, 2023 19:30

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2023

tekton-robot requested review from imjasonh and jerop February 13, 2023 19:30

kyubisation approved these changes Feb 13, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/apply.go Outdated Show resolved Hide resolved

jerop reviewed Feb 13, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/apply.go Outdated Show resolved Hide resolved

jerop reviewed Feb 13, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/apply.go Outdated Show resolved Hide resolved

jerop reviewed Feb 13, 2023

View reviewed changes

pkg/reconciler/pipelinerun/resources/apply.go Outdated Show resolved Hide resolved

Yongxuanzhang force-pushed the fix-6139 branch from b122f4d to 5847110 Compare February 13, 2023 21:04

Yongxuanzhang changed the title ~~don't validate skipped task resutls for pipeline results~~ don't validate skipped task results for pipeline results Feb 13, 2023

Yongxuanzhang requested review from jerop and removed request for bobcatfish, imjasonh and lbernick February 13, 2023 21:04

jerop reviewed Feb 13, 2023

View reviewed changes

pritidesai added this to the Pipelines v0.46 milestone Mar 6, 2023

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Mar 6, 2023

Yongxuanzhang force-pushed the fix-6139 branch from bc4ef61 to 72a556d Compare March 10, 2023 18:58

lbernick reviewed Mar 15, 2023

View reviewed changes

Yongxuanzhang force-pushed the fix-6139 branch from 2ba7866 to e010e87 Compare March 16, 2023 03:58

afrittoli reviewed Mar 16, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2023

tekton-robot assigned lbernick Mar 16, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2023

tekton-robot merged commit d89850f into tektoncd:main Mar 16, 2023

	received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, nil, logtesting.TestLogger(t))
	received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, nil /* skippedTasks */, logtesting.TestLogger(t))

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

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

Uh oh!

Conversation

Yongxuanzhang commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Feb 13, 2023

Uh oh!

tekton-robot commented Feb 13, 2023

Uh oh!

tekton-robot commented Feb 13, 2023

Uh oh!

Yongxuanzhang commented Feb 13, 2023

Uh oh!

kyubisation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerop commented Feb 13, 2023

Uh oh!

jerop left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Feb 13, 2023

Uh oh!

tekton-robot commented Feb 13, 2023

Uh oh!

Yongxuanzhang commented Feb 13, 2023

Uh oh!

tekton-robot commented Mar 6, 2023

Uh oh!

pritidesai commented Mar 6, 2023

Uh oh!

tekton-robot commented Mar 10, 2023

Uh oh!

tekton-robot commented Mar 10, 2023

Uh oh!

lbernick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lbernick Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Yongxuanzhang Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

lbernick Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

lbernick Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

Yongxuanzhang Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Mar 16, 2023

Uh oh!

tekton-robot commented Mar 16, 2023

Uh oh!

tekton-robot commented Mar 16, 2023

Uh oh!

tekton-robot commented Mar 16, 2023

Uh oh!

tekton-robot commented Mar 16, 2023

Uh oh!

tekton-robot commented Mar 16, 2023

Uh oh!

tekton-robot commented Mar 16, 2023

Uh oh!

afrittoli left a comment

Choose a reason for hiding this comment

Uh oh!

Yongxuanzhang commented Feb 13, 2023 •

edited

Loading