TEP-0048: default results - #9653
Conversation
05d242d to
7996ae5
Compare
|
/assign |
afrittoli
left a comment
There was a problem hiding this comment.
Thank you Priti for this, it would be great to have this feature!
And thanks for dealing with the matrix case too.
The only real concern I have is with the change of signature of exported functions and inconsistent use of context there, I made a suggestion which I think will simplify things (reduce changes) and fix the issue.
I suggested a couple of improvements with helper functions as well.
BTW, do you plan on implementing default results for pipelines in case of skipped tasks as well?
| for _, taskResult := range resolvedTask.TaskSpec.Results { | ||
| if taskResult.Name == reference.Result && taskResult.Default != nil { | ||
| return *taskResult.Default, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop is used in a few places, perhaps it's worth factoring it up to a private function, e.g.
// taskResultDefault returns the default value for resultName from the TaskSpec,
// or nil if none is declared.func taskResultDefault(spec *v1.TaskSpec, resultName string) *v1.ResultValue {
for _, r := range spec.Results {
if r.Name == resultName {
return r.Default // nil if not set
}
}
return nil
}There was a problem hiding this comment.
I think this is resolved now 🤔
| if referencedPipelineTask.PipelineTask.IsMatrixed() { | ||
| // For a matrixed task that didn't run, wrap the default into array format. | ||
| var arrayVal v1.ParamValue | ||
| if taskResult.Default.Type == v1.ParamTypeArray { | ||
| arrayVal = *taskResult.Default | ||
| } else { | ||
| arrayVal = v1.ParamValue{Type: v1.ParamTypeArray, ArrayVal: []string{taskResult.Default.StringVal}} | ||
| } |
There was a problem hiding this comment.
You could use here the helper matrixedDefaultValue from my other comment and fix the behaviour for object params - again, it's probably blocked by validation, but safer to handle here as well.
There was a problem hiding this comment.
I am using the matrixDefaultValue now from your other comment
279dffb to
beafe5a
Compare
waveywaves
left a comment
There was a problem hiding this comment.
Adding to afrittoli's review with two additional observations:
-
v1beta1 conversion gap:
result_conversion.godoes not convert the newDefaultfield between v1 and v1beta1. A v1 Task with a default result, converted to v1beta1 and back, will silently lose the default. The v1beta1TaskResultstruct needs theDefaultfield added, or the conversion must explicitly handle it. -
Missing 'produced overrides default' test: All test cases in
TestApplyTaskResultsToPipelineResults_DefaultResultsverify default-when-missing scenarios. The most critical invariant (actual produced result takes precedence over default) appears untested. Adding one test case where a task produces a result AND has a default would prove this works correctly.
Also needs rebase.
|
@copilot resolve the merge conflicts in this pull request |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@pritidesai it needs a rebase 🙏🏼 |
Hi @waveywaves, along with the conversion: Is there anything specific I might have missed? |
There is a test case which validates use of an actual result even with default defined: Does this work or should we add some more tests? |
|
@afrittoli @vdemeester @waveywaves thank you for your input. I have addressed the feedback, and the PR is now ready for review 🙏 |
There was a problem hiding this comment.
Pull request overview
Adds support for default Task results (TEP-0048) behind the enable-default-results alpha feature flag, allowing pipelines to resolve missing task results using declared defaults (including when tasks are skipped). It also introduces validation to fail TaskRuns that don’t emit non-defaulted declared results.
Changes:
- Introduce
defaulton Task result declarations (v1 + v1beta1) with API/schema/codegen/CRD updates and validation gated byenable-default-results. - Populate/use default result values across TaskRun reconciliation and PipelineRun result reference resolution (including skipped tasks and matrix cases).
- Add docs + examples and extend unit/e2e plumbing to exercise alpha examples with the feature enabled.
Reviewed changes
Copilot reviewed 34 out of 38 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e-tests.sh | Adds env var + configmap patching to toggle enable-default-results for alpha example runs. |
| pkg/reconciler/taskrun/validate_taskrun.go | Adds validation to ensure declared non-default results are produced (flag-gated). |
| pkg/reconciler/taskrun/validate_taskrun_test.go | Unit tests for the new “missing non-default results” validation behavior. |
| pkg/reconciler/taskrun/taskrun.go | Pre-populates default results during prepare and invokes results-produced validation during reconcile. |
| pkg/reconciler/taskrun/taskrun_test.go | Controller test verifying default result pre-population behavior. |
| pkg/reconciler/pipelinerun/resources/resultrefresolution.go | Uses declared defaults when resolving missing referenced results (incl. skipped tasks + matrix). |
| pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go | Adds tests covering result-ref resolution using default values. |
| pkg/reconciler/pipelinerun/resources/pipelinerunstate.go | Stores EnableDefaultResults in facts; adds helper to extract task result defaults from resolved tasks. |
| pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go | Tests GetTaskResultDefaults; updates GetChildReferences call signature. |
| pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go | Threads enableDefaultResults through result-ref checks; resolves TaskSpec even when TaskRun absent. |
| pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go | Adds finally-skip behavior tests when defaults exist + flag enabled. |
| pkg/reconciler/pipelinerun/resources/apply.go | Enables PipelineRun result population from declared task result defaults (flag-gated). |
| pkg/reconciler/pipelinerun/resources/apply_test.go | Updates callers for new signature and adds coverage for default-based pipeline results. |
| pkg/reconciler/pipelinerun/pipelinerun.go | Wires feature flag into facts; passes defaults+ctx into pipeline results application and result-ref checks. |
| pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go | DeepCopy support for new TaskResult.Default. |
| pkg/apis/pipeline/v1beta1/task_conversion_test.go | Conversion test ensuring default round-trips between v1beta1 and v1. |
| pkg/apis/pipeline/v1beta1/swagger.json | Swagger schema includes default for v1beta1 task results. |
| pkg/apis/pipeline/v1beta1/result_validation.go | Adds validation for default (type + properties) and requires feature flag enabled. |
| pkg/apis/pipeline/v1beta1/result_types.go | Adds Default field to v1beta1 TaskResult. |
| pkg/apis/pipeline/v1beta1/result_conversion.go | Converts Default between v1beta1 and v1. |
| pkg/apis/pipeline/v1beta1/openapi_generated.go | OpenAPI includes default for v1beta1 TaskResult. |
| pkg/apis/pipeline/v1/zz_generated.deepcopy.go | DeepCopy support for new TaskResult.Default. |
| pkg/apis/pipeline/v1/swagger.json | Swagger schema includes default for v1 task results. |
| pkg/apis/pipeline/v1/result_validation.go | Adds validation for default (type + properties) and requires feature flag enabled. |
| pkg/apis/pipeline/v1/result_validation_test.go | Unit tests for default validation (flag gating, type checks, object keys). |
| pkg/apis/pipeline/v1/result_types.go | Adds Default field to v1 TaskResult. |
| pkg/apis/pipeline/v1/openapi_generated.go | OpenAPI includes default for v1 TaskResult. |
| pkg/apis/config/feature_flags.go | Introduces enable-default-results feature flag (alpha) and parses it into config. |
| examples/v1/taskruns/alpha/default-results.yaml | New alpha TaskRun example demonstrating defaults for string/array/object results. |
| examples/v1/pipelineruns/beta/pipelinerun-with-matrix-context-variables.yaml | Adjusts example logic to match expected matrix results behavior. |
| examples/v1/pipelineruns/alpha/pipelinerun-with-default-results.yaml | New alpha PipelineRun example using defaults across tasks + finally (incl. skipped task). |
| examples/v1/pipelineruns/alpha/pipelinerun-emitting-default-results.yaml | New alpha PipelineRun example showing pipeline results populated via task defaults. |
| docs/tasks.md | Documents default result values and feature-flag gating under the Results section. |
| docs/pipeline-api.md | API reference docs updated to include default on Task results (v1 + v1beta1). |
| config/config-feature-flags.yaml | Adds enable-default-results entry to feature-flags ConfigMap template. |
| config/300-crds/300-taskrun.yaml | CRD schema updates to include the new feature flag field and result default field. |
| config/300-crds/300-task.yaml | CRD schema updates to include Task result default. |
| config/300-crds/300-pipelinerun.yaml | CRD schema updates to include the new feature flag field. |
Files not reviewed (4)
- pkg/apis/pipeline/v1/openapi_generated.go: Generated file
- pkg/apis/pipeline/v1/zz_generated.deepcopy.go: Generated file
- pkg/apis/pipeline/v1beta1/openapi_generated.go: Generated file
- pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a94df1e to
b8836a6
Compare
|
@vdemeester I’ve addressed all of Copilot’s review comments. Are there any additional changes you think we should consider? |
This commit introduces default results, allowing task authors to specify a default value for a result. If a task does not produce a value for that result during execution, the specified default will be used. This ensures that downstream tasks referencing that result can still resolve params consuming the results, preventing unncessary failures in the pipeline. With this change, if a task author does not define a default value and none of the steps produce the corresponding result, the entire taskRun will be marked as failed. For more details, please refer to TEP-0048 Co-authored-by: Vinamra Jain <vinjain@redhat.com> Assisted-by: Claude 4.6 Sonnet Signed-off-by: Priti Desai <pdesai@us.ibm.com>
Changes
This commit introduces default results, allowing task authors to specify a default value for a result. If a task does not produce a value for that result during execution, the specified default will be used. This ensures that downstream tasks referencing that result can still resolve params consuming the results, preventing unnecessary failures in the pipeline.
With this change, if a task author does not define a default value and none of the steps produce the corresponding result, the entire taskRun will be marked as failed.
For more details, please refer to TEP-0048
The changes in this PR are based on #5620 authored by @vinamra28.
Co-authored-by: Vinamra Jain vinjain@redhat.com
Assisted-by: Claude Sonnet 4.6
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes