[TEP-0076] Add array support for emitting results #4818

Yongxuanzhang · 2022-05-02T18:29:54Z

Changes

This is part of work in TEP-0076
This commit provides support for emitting array results. Previous to
this commit we have changed the TaskResults Type to support array. But we can only emit string type result.

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

Support array type for emitting results from a task as an alpha feature.

The type of the result is changed from string to ArrayOrString.

A task can specify a type to produce array result, such as:

  results:
    - name: array-results
      type: array
      description: The array results

And the task script can populate result in an array form with:

echo -n "[\"hello\",\"world\"]" | tee $(results.array-results.path)

 This feature is part of the TEP-0076 and its in progress to index into the array result while consuming that result.

pkg/apis/pipeline/v1beta1/resource_types.go

tekton-robot · 2022-05-02T18:32:41Z

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/entrypoint/entrypointer.go	69.7%	66.7%	-3.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	95.1%	94.0%	-1.1

tekton-robot · 2022-05-02T18:52:11Z

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/entrypoint/entrypointer.go	69.7%	66.7%	-3.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	95.1%	94.0%	-1.1

tekton-robot · 2022-05-02T19:06:14Z

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/entrypoint/entrypointer.go	69.7%	66.7%	-3.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	95.1%	94.0%	-1.1

tekton-robot · 2022-05-02T20:04:36Z

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/entrypoint/entrypointer.go	69.7%	66.7%	-3.0
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	95.1%	94.0%	-1.1

tekton-robot · 2022-05-02T21:08:36Z

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/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

Yongxuanzhang · 2022-05-02T21:10:18Z

pkg/entrypoint/entrypointer.go

 			logger.Fatalf("Error while handling results: %s", err)
 		}
 	}

 	return err
 }

-func (e Entrypointer) readResultsFromDisk() error {
+func (e Entrypointer) readResultsFromDisk(resultDir string) error {


This is for better unit testing. Without this I think we cannot create "tekton/results" dir to emit results

tekton-robot · 2022-05-02T21:21:37Z

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/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

tekton-robot · 2022-05-02T21:38:03Z

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/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

tekton-robot · 2022-05-02T21:56: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/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

tekton-robot · 2022-05-02T23:12:24Z

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/apis/pipeline/v1beta1/param_types.go	98.1%	94.7%	-3.4
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

tekton-robot · 2022-05-03T00:22:33Z

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/apis/pipeline/v1beta1/param_types.go	98.1%	98.3%	0.2
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

tekton-robot · 2022-05-03T01:03:34Z

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/apis/pipeline/v1beta1/param_types.go	98.1%	98.3%	0.2
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

tekton-robot · 2022-05-03T13:49:48Z

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/apis/pipeline/v1beta1/param_types.go	98.1%	98.3%	0.2
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

ywluogg · 2022-05-03T14:00:31Z

/assign ywluogg

tekton-robot · 2022-05-03T14: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/apis/pipeline/v1beta1/param_types.go	98.1%	98.3%	0.2
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

pkg/apis/pipeline/v1beta1/param_types.go

pkg/apis/pipeline/v1beta1/param_types_test.go

tekton-robot · 2022-05-03T14:25:31Z

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/apis/pipeline/v1beta1/param_types.go	98.1%	98.3%	0.2
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	95.2%	1.4

ywluogg · 2022-05-03T14:37:48Z

pkg/apis/pipeline/v1beta1/result_types.go

@@ -39,7 +39,7 @@ type TaskRunResult struct {
 	Type ResultsType `json:"type,omitempty"`

 	// Value the given value of the result
-	Value string `json:"value"`
+	Value ArrayOrString `json:"value"`


This looks like a API change to me. It seems like we need to follow https://github.com/tektoncd/pipeline/blob/main/api_compatibility_policy.md#alpha-beta-and-ga for adding new feature and deprecation rules. Pinging @dibyom for confirmation. This is a field that users can specify. I think we should add a separate field for the new ArrayOrString and implement on the new field

It is a change, but it won't have effect on users or introduce breaking changes. And our TEP0076 requires us to change this Value I believe?

We came to a conclusion that this won't be breaking the API change policy. However, we definitely are blocked by the design about whether we want to have a brand new struct on this or just rename StringOrArray.

Ok but it doesn't block this PR right, I'm still using StringOrArray. We need to figure out that in Chuang's

cc:@ywluogg, I remember you send email to remind the changes, so we should send emails specifically about cli for this?

It looks like api mismatch between the pipeline and cli, it should be fixed when we release this change and then bump the pipeline version for cli? Am I correct? @pritidesai

/cc @tektoncd/cli-maintainers

It looks like api mismatch between the pipeline and cli, it should be fixed when we release this change and then bump the pipeline version for cli? Am I correct? @pritidesai

I think It will be fixed when CLI bumps the pipelines version (which will have these changes) and update their usage/implementation to change the type of the results from string to StringOrArray. The error reported here is coming from:

./pkg/cmd/taskrun/list.go: return fmt.Errorf("failed to list TaskRuns from namespace %s: %v", p.Namespace(), err)

tekton-robot · 2022-05-03T15:35: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/apis/pipeline/v1beta1/param_types.go	98.1%	98.3%	0.2
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

tekton-robot · 2022-05-03T15:37:33Z

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/apis/pipeline/v1beta1/param_types.go	98.1%	98.3%	0.2
pkg/entrypoint/entrypointer.go	69.7%	84.1%	14.4
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

Yongxuanzhang · 2022-05-03T16:40:27Z

/test pull-tekton-pipeline-build-tests

tekton-robot · 2022-05-20T21:51:14Z

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/apis/pipeline/v1beta1/param_types.go	96.9%	97.4%	0.5
pkg/entrypoint/entrypointer.go	69.7%	84.8%	15.2
pkg/pod/status.go	91.2%	91.1%	-0.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

pritidesai · 2022-05-24T21:47:27Z

pkg/reconciler/pipelinerun/resources/resultrefresolution.go

 	var err error
 	if referencedPipelineTask.IsCustomTask() {
 		runName = referencedPipelineTask.Run.Name
-		resultValue, err = findRunResultForParam(referencedPipelineTask.Run, resultRef)
+		runValue, err = findRunResultForParam(referencedPipelineTask.Run, resultRef)
+		resultValue = *v1beta1.NewArrayOrString(runValue)


should this be calling unmarshalling here? Calling NewArrayOrString with runValue here returns a string.

Also, why limiting this to just the custom task?

Also, why limiting this to just the custom task?

So custom task results are still string:

type RunResult struct { // Name the given name Name string `json:"name"` // Value the given value of the result Value string `json:"value"` }

Are we planning on changing the custom task result to string/array? Something coming up ...

Yes, this is a great question! We don't support structured results for custom task in this PR. So this is intentionally to return string here.

We plan to add support for task and pipeline first, custom task is not discussed and also not explicitly mentioned in the tep (in tep we only mention task and pipeline) but we will discuss later!

https://github.com/tektoncd/community/blob/main/teps/0076-array-result-types.md#summary

The first use case explicitly talks about custom task - https://github.com/tektoncd/community/blob/main/teps/0076-array-result-types.md#use-cases Custom task params has use case to consume an array which could be a result of other task and produce a result with an array (a list of hashes/a list of built images).

Thank you @Yongxuanzhang for following up on this 🙏

Yes! I have added in our tracking issue

tekton-robot · 2022-05-24T21:56:06Z

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/apis/pipeline/v1beta1/param_types.go	96.9%	97.4%	0.5
pkg/entrypoint/entrypointer.go	69.7%	84.8%	15.2
pkg/pod/status.go	91.2%	90.9%	-0.3
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

This commit provides support for emitting array results via changing TaskRunResult value from string to ArrayorString and add ResultValue for PipelineResourceResult. Previous to this commit we can only emit string type result.

pritidesai · 2022-05-25T00:02:36Z

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-go-coverage

tekton-robot · 2022-05-25T00:07: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/apis/pipeline/v1beta1/param_types.go	96.9%	97.4%	0.5
pkg/entrypoint/entrypointer.go	69.7%	84.8%	15.2
pkg/pod/status.go	91.2%	90.9%	-0.3
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	93.8%	94.0%	0.1

pritidesai · 2022-05-25T00:09:35Z

@Yongxuanzhang thank you for this 🙏 I have updated the release notes.

Yongxuanzhang · 2022-05-25T00:23:21Z

@Yongxuanzhang thank you for this 🙏 I have updated the release notes.

Thank you!! ❤️

pritidesai · 2022-05-25T03:49:23Z

We have opened an issue to track a type change from string to StringOrArray. This might impact CLI and anyone consuming the result type directly.

This change will be part of the GitHub release notes after a release is created.

/lgtm

This commit fixes the string result type validation, PR tektoncd#4779 adds result type and asumes that the default type should be string via mutating webhook. PR tektoncd#4818 adds the validation for this. However, resources that did already exist in etcd didn't get the default. So this commit relaxes this validation.

This commit fixes the string result type validation, PR tektoncd#4779 adds result type and asumes that the default type should be string via mutating webhook. PR tektoncd#4818 adds the validation for this. However, resources that did already exist in etcd didn't get the default. So this commit relaxes the validation for empty result type.

This commit fixes the string result type validation, PR #4779 adds result type and asumes that the default type should be string via mutating webhook. PR #4818 adds the validation for this. However, resources that did already exist in etcd didn't get the default. So this commit relaxes the validation for empty result type.

This commit fixes the string result type validation, PR tektoncd#4779 adds result type and asumes that the default type should be string via mutating webhook. PR tektoncd#4818 adds the validation for this. However, resources that did already exist in etcd didn't get the default. So this commit relaxes the validation for empty result type. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

This commit fixes the string result type validation, PR #4779 adds result type and asumes that the default type should be string via mutating webhook. PR #4818 adds the validation for this. However, resources that did already exist in etcd didn't get the default. So this commit relaxes the validation for empty result type. Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>

This commit fixes the string result type validation, PR tektoncd#4779 adds result type and asumes that the default type should be string via mutating webhook. PR tektoncd#4818 adds the validation for this. However, resources that did already exist in etcd didn't get the default. So this commit relaxes the validation for empty result type.

This commit fixes the string result type validation, PR #4779 adds result type and asumes that the default type should be string via mutating webhook. PR #4818 adds the validation for this. However, resources that did already exist in etcd didn't get the default. So this commit relaxes the validation for empty result type.

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 2, 2022

tekton-robot requested review from dibyom and imjasonh May 2, 2022 18:30

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2022

Yongxuanzhang commented May 2, 2022

View reviewed changes

pkg/apis/pipeline/v1beta1/resource_types.go Outdated Show resolved Hide resolved

Yongxuanzhang commented May 2, 2022

View reviewed changes

tekton-robot assigned ywluogg May 3, 2022

ywluogg reviewed May 3, 2022

View reviewed changes

pkg/apis/pipeline/v1beta1/param_types.go Show resolved Hide resolved

pkg/apis/pipeline/v1beta1/param_types_test.go Outdated Show resolved Hide resolved

ywluogg reviewed May 3, 2022

View reviewed changes

Yongxuanzhang marked this pull request as ready for review May 3, 2022 15:51

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 May 3, 2022

Yongxuanzhang force-pushed the support-array-results branch from 2f6547a to 920c5e5 Compare May 20, 2022 21:47

Yongxuanzhang mentioned this pull request May 24, 2022

[Breaking Change] Change TaskRunResult.Value and PipelineRunResult.Value from string to ArrayOrString #4907

Closed

pritidesai reviewed May 24, 2022

View reviewed changes

Yongxuanzhang force-pushed the support-array-results branch from 920c5e5 to a13d9d4 Compare May 24, 2022 21:51

[TEP-0076] Add array support for emitting results

afacd43

This commit provides support for emitting array results via changing TaskRunResult value from string to ArrayorString and add ResultValue for PipelineResourceResult. Previous to this commit we can only emit string type result.

Yongxuanzhang force-pushed the support-array-results branch from a13d9d4 to afacd43 Compare May 24, 2022 22:06

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2022

tekton-robot merged commit d389ed2 into tektoncd:main May 25, 2022

ywluogg mentioned this pull request Jun 22, 2022

Add ywluogg into Pipeline reviewer roles tektoncd/community#737

Merged

Yongxuanzhang mentioned this pull request Jun 27, 2022

Relax result type validation to avoid nightly build failure #5043

Merged

5 tasks

afrittoli mentioned this pull request Jul 1, 2022

Relax result type validation to avoid nightly build failure #5065

Merged

5 tasks

afrittoli mentioned this pull request Jul 4, 2022

Relax result type validation to avoid nightly build failure #5068

Merged

5 tasks

lbernick mentioned this pull request Jul 21, 2022

Restore periodic prow job for upgrade tests #5193

Closed

Yongxuanzhang mentioned this pull request Sep 27, 2022

Mark tep75&76 implemented tektoncd/community#833

Merged

[TEP-0076] Add array support for emitting results #4818

[TEP-0076] Add array support for emitting results #4818

Conversation

Yongxuanzhang commented May 2, 2022 • edited by pritidesai

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 2, 2022

tekton-robot commented May 2, 2022

tekton-robot commented May 2, 2022

tekton-robot commented May 2, 2022

tekton-robot commented May 2, 2022

Choose a reason for hiding this comment

tekton-robot commented May 2, 2022

tekton-robot commented May 2, 2022

tekton-robot commented May 2, 2022

tekton-robot commented May 2, 2022

tekton-robot commented May 3, 2022

tekton-robot commented May 3, 2022

tekton-robot commented May 3, 2022

ywluogg commented May 3, 2022

tekton-robot commented May 3, 2022

tekton-robot commented May 3, 2022

Choose a reason for hiding this comment

Yongxuanzhang May 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang May 24, 2022 • edited

Choose a reason for hiding this comment

Yongxuanzhang May 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 3, 2022

tekton-robot commented May 3, 2022

Yongxuanzhang commented May 3, 2022

tekton-robot commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 24, 2022

pritidesai commented May 25, 2022

tekton-robot commented May 25, 2022

pritidesai commented May 25, 2022

Yongxuanzhang commented May 25, 2022

pritidesai commented May 25, 2022

Yongxuanzhang commented May 2, 2022 •

edited by pritidesai

Yongxuanzhang May 3, 2022 •

edited

Yongxuanzhang May 24, 2022 •

edited

Yongxuanzhang May 24, 2022 •

edited