[TEP-0076]Add type for results #4779

Yongxuanzhang · 2022-04-20T14:53:02Z

Changes

This commit is the first step of TEP-0076, to support array and object
in resutls we need to add type for TaskResult and TaskRunResult first.
Before this commit we don't have the Type for these results.

Besides, for we move the TaskResult and TaskRunResult into the centralized result_types.go, so it will be consistent with pararm_types.go

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

Add Type for TaskRunResult and TaskResult.

tekton-robot · 2022-04-20T15:03:10Z

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/task_defaults.go	80.0%	88.9%	8.9
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.1%	0.0

Yongxuanzhang · 2022-04-20T15:04:43Z

@ywluogg is this the right direction for first step?

tekton-robot · 2022-04-20T15:38:53Z

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/task_defaults.go	80.0%	88.9%	8.9
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.1%	0.0

Yongxuanzhang · 2022-04-20T16:04:55Z

/assign @ywluogg

tekton-robot · 2022-04-20T16:04:57Z

@Yongxuanzhang: GitHub didn't allow me to assign the following users: ywluogg.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @ywluogg

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/apis/pipeline/v1beta1/swagger.json

pkg/apis/pipeline/v1beta1/results.go

ywluogg · 2022-04-20T21:24:49Z

pkg/apis/pipeline/v1beta1/results.go

+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.


I see params for the similar file is called params_types.go. Maybe it worths being consistent with params?

Yes this is also something I'm not sure. I thought files ended with "_types" should have a crd in it.

That's a pretty good consideration. I think TaskResult is actually similar to ParamsSpec:

pipeline/pkg/apis/pipeline/v1beta1/param_types.go

Line 37 in 7e5e4c0

type ParamSpec struct {

and TaskRunResult is similar to Param:

pipeline/pkg/apis/pipeline/v1beta1/param_types.go

Line 83 in 7e5e4c0

type Param struct {

Maybe we can move those structs into a centralized file results_types.go.

yeah that would also work, I can do it and see how other ppl think of it

pkg/apis/pipeline/v1beta1/taskrun_types.go

ywluogg · 2022-04-20T21:55:46Z

pkg/apis/pipeline/v1beta1/results.go

+
+// ResultsType indicates the type of a result;
+// Used to distinguish between a single string and an array of strings.
+type ResultsType string


Maybe it should start having an interface for ResultType and ParamsType, as they ultimately need to be in ArrayOrString:

pipeline/pkg/apis/pipeline/v1beta1/param_types.go

Line 106 in 983dc37

type ArrayOrString struct {

Pinging others @dibyom @vdemeester for more opinions

Any reason for not moving them to ArrayOrString "now", as, from the API perspective (consumption) it doesn't change too much, all current API usage will still work because string is still supported.

Good point! I would suggest to add a TODO for changing this to have ResultType also in scope

tekton-robot · 2022-04-20T22:26:19Z

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/task_defaults.go	80.0%	88.9%	8.9
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.1%	0.0

tekton-robot · 2022-04-20T22:38:54Z

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/task_defaults.go	80.0%	88.9%	8.9
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.1%	0.0

tekton-robot · 2022-04-21T13:58:27Z

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/result_defaults.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/result_validation.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/task_defaults.go	80.0%	85.7%	5.7
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.0%	-0.1

pkg/apis/pipeline/v1beta1/result_types.go

tekton-robot · 2022-04-21T18:11:59Z

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/result_defaults.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/result_validation.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/task_defaults.go	80.0%	85.7%	5.7
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.0%	-0.1

pkg/apis/pipeline/v1beta1/result_defaults.go

Yongxuanzhang · 2022-04-21T18:39:58Z

/test pull-tekton-pipeline-integration-tests

ywluogg · 2022-04-21T19:28:02Z

/test pull-tekton-pipeline-integration-tests

Yongxuanzhang · 2022-04-21T22:30:53Z

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

Yongxuanzhang · 2022-04-22T13:57:04Z

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

tekton-robot · 2022-04-22T14:54:59Z

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/result_defaults.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/result_validation.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/task_defaults.go	80.0%	85.7%	5.7
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.0%	-0.1

tekton-robot · 2022-04-27T22:18:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, ywluogg

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [dibyom]

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

tekton-robot · 2022-04-28T15:05: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/result_defaults.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/result_validation.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/task_defaults.go	80.0%	85.7%	5.7
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.0%	-0.1

Yongxuanzhang · 2022-04-28T15:40:32Z

/test pull-tekton-pipeline-integration-tests

Yongxuanzhang · 2022-04-28T17:01:30Z

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

Yongxuanzhang · 2022-04-28T18:43:49Z

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

pkg/apis/pipeline/v1beta1/result_types.go

lbernick · 2022-04-29T14:00:37Z

pkg/apis/pipeline/v1beta1/result_validation.go

+	}
+	// Validate the result type
+	validType := false
+	for _, allowedType := range AllResultsTypes {


nit: we've used sets.String elsewhere in the codebase, might be helpful here https://pkg.go.dev/k8s.io/apimachinery@v0.23.6/pkg/util/sets

yeah I was using map before, and I see param type is using a list. 😄 I will change it to sets.String

emmm maybe I'm wrong, looks like I cannot use sets.String here, because sets.String is using string as key but we're using ResultsType

The following code will throw error
var AllResultsTypes = sets.String{ResultsTypeString:{}}

This commit is the first step of TEP-0076, to support array and object in resutls we need to add type for TaskResult and TaskRunResult first. Before this commit we don't have the Type for these results.

tekton-robot · 2022-04-29T20:43:26Z

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/result_defaults.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/result_validation.go	Do not exist	100.0%
pkg/apis/pipeline/v1beta1/task_defaults.go	80.0%	85.7%	5.7
pkg/apis/pipeline/v1beta1/task_validation.go	97.1%	97.0%	-0.1

lbernick · 2022-04-29T20:47:25Z

/lgtm

Yongxuanzhang · 2022-04-29T21:26:58Z

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

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 requested review from dibyom and vdemeester April 20, 2022 14:53

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 20, 2022

ywluogg reviewed Apr 20, 2022

View reviewed changes

Yongxuanzhang commented Apr 21, 2022

View reviewed changes

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

Yongxuanzhang commented Apr 21, 2022

View reviewed changes

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

Yongxuanzhang marked this pull request as ready for review April 21, 2022 19:15

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 Apr 21, 2022

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 22, 2022

Yongxuanzhang force-pushed the add-results-type branch from 5736fec to 3e10239 Compare April 22, 2022 14:57

tekton-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 22, 2022

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2022

Yongxuanzhang force-pushed the add-results-type branch from d044d78 to 4b30812 Compare April 28, 2022 15:03

lbernick reviewed Apr 29, 2022

View reviewed changes

[TEP-0076]Add type for results

c8f3fb5

This commit is the first step of TEP-0076, to support array and object in resutls we need to add type for TaskResult and TaskRunResult first. Before this commit we don't have the Type for these results.

Yongxuanzhang force-pushed the add-results-type branch from 4b30812 to c8f3fb5 Compare April 29, 2022 20:40

tekton-robot assigned lbernick Apr 29, 2022

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

tekton-robot merged commit 6f39e94 into tektoncd:main Apr 29, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0076]Add type for results #4779

[TEP-0076]Add type for results #4779

Yongxuanzhang commented Apr 20, 2022 •

edited

Loading

tekton-robot commented Apr 20, 2022

Yongxuanzhang commented Apr 20, 2022

tekton-robot commented Apr 20, 2022

Yongxuanzhang commented Apr 20, 2022

tekton-robot commented Apr 20, 2022

ywluogg Apr 20, 2022

Yongxuanzhang Apr 20, 2022

ywluogg Apr 20, 2022

Yongxuanzhang Apr 20, 2022

ywluogg Apr 20, 2022

vdemeester Apr 21, 2022

ywluogg Apr 21, 2022

tekton-robot commented Apr 20, 2022

tekton-robot commented Apr 20, 2022

tekton-robot commented Apr 21, 2022

tekton-robot commented Apr 21, 2022

Yongxuanzhang commented Apr 21, 2022

ywluogg commented Apr 21, 2022

Yongxuanzhang commented Apr 21, 2022

Yongxuanzhang commented Apr 22, 2022

tekton-robot commented Apr 22, 2022

tekton-robot commented Apr 27, 2022

tekton-robot commented Apr 28, 2022

Yongxuanzhang commented Apr 28, 2022

Yongxuanzhang commented Apr 28, 2022

Yongxuanzhang commented Apr 28, 2022

lbernick Apr 29, 2022

Yongxuanzhang Apr 29, 2022

Yongxuanzhang Apr 29, 2022

tekton-robot commented Apr 29, 2022

lbernick commented Apr 29, 2022

Yongxuanzhang commented Apr 29, 2022

[TEP-0076]Add type for results #4779

[TEP-0076]Add type for results #4779

Conversation

Yongxuanzhang commented Apr 20, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 20, 2022

Yongxuanzhang commented Apr 20, 2022

tekton-robot commented Apr 20, 2022

Yongxuanzhang commented Apr 20, 2022

tekton-robot commented Apr 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

Choose a reason for hiding this comment

tekton-robot commented Apr 20, 2022

tekton-robot commented Apr 20, 2022

tekton-robot commented Apr 21, 2022

tekton-robot commented Apr 21, 2022

Yongxuanzhang commented Apr 21, 2022

ywluogg commented Apr 21, 2022

Yongxuanzhang commented Apr 21, 2022

Yongxuanzhang commented Apr 22, 2022

tekton-robot commented Apr 22, 2022

tekton-robot commented Apr 27, 2022

tekton-robot commented Apr 28, 2022

Yongxuanzhang commented Apr 28, 2022

Yongxuanzhang commented Apr 28, 2022

Yongxuanzhang commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 29, 2022

lbernick commented Apr 29, 2022

Yongxuanzhang commented Apr 29, 2022

Yongxuanzhang commented Apr 20, 2022 •

edited

Loading