[TEP-0075] Validate object keys, PipelineRunSpec -> PipelineSpec #4883

chuangw6 · 2022-05-18T05:45:55Z

Changes

Add validation that checks if object param value from PipelineRunSpec
misses some keys required for that object param declared in PipelineSpec.

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

Make sure keys of an object param declared in PipelineSpec are all provided with a value when the PipelineRunSpec provides values for the object param. In other words, the value provider - PipelineRunSpec can't miss keys. i.e. the following example will be invalid since the key `commit` is missed.

Example:
-----------
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: test-case
spec:
  params:
  - name: myObject
    value:
      url: "abc.com"
  pipelineSpec:
    params:
    - name: myObject
      properties:
        url: {}
        commit: {}

tekton-robot · 2022-05-18T05:49:46Z

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/pipelinerun.go	86.2%	86.2%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	84.6%	-15.4

tekton-robot · 2022-05-18T06:02: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/reconciler/pipelinerun/pipelinerun.go	86.2%	86.2%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	84.6%	-15.4

chuangw6 · 2022-05-20T04:09:18Z

/assign @ywluogg

tekton-robot · 2022-05-20T04:09:20Z

@chuangw6: 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.

ywluogg · 2022-06-02T18:12:42Z

/assign @ywluogg

ywluogg · 2022-06-02T18:13:17Z

/assign @ywluogg

I feel like this is a bug since I can get notification from Yongxuan's assignment but not from yours

ywluogg · 2022-06-02T18:18:24Z

Link this to #4723

ywluogg

Nice!

tekton-robot · 2022-06-09T20:35:20Z

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/pipelinerun.go	86.1%	86.2%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	84.6%	-15.4

chuangw6 · 2022-06-09T21:32:41Z

/test pull-tekton-pipeline-integration-tests

dibyom

Thanks @chuangw6 Could you add a release note?

tekton-robot · 2022-06-14T22:03:16Z

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/pipelinerun.go	86.1%	86.2%	0.0
pkg/reconciler/pipelinerun/resources/validate_params.go	100.0%	84.6%	-15.4

chuangw6 · 2022-06-14T22:30:34Z

Thanks @chuangw6 Could you add a release note?

Added! Thanks @dibyom

Also added more tests to get coverage rate back 😃

tekton-robot · 2022-06-14T22:31: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/reconciler/pipelinerun/pipelinerun.go	86.1%	86.2%	0.0

tekton-robot · 2022-06-14T22:44:39Z

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/pipelinerun.go	86.1%	86.2%	0.0

lbernick · 2022-06-15T14:51:03Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+		permanentError: true,
+		wantEvents: []string{
+			"Normal Started",
+			"Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing some object keys",


This error message would be more helpful if it returned which keys were missing and for which parameters

sg! Changed the verifier helper function to return a map (key: param, val: missing keys) so when the users see the err message from pipelinerun status, they will see which keys were missing and for which parameters.

But for the wantEvent in testing, if I also put the returned err message here, it gives weird error and fails test though the expected and got event reported from the following message are exactly same. I guess this is why other wantEvent cuts off the second half message too.

logger.go:130: 2022-06-15T11:43:08.897-0700 ERROR TestReconcile_InvalidPipelineRuns/invalid-pipeline-missing-object-keys pipelinerun/reconciler.go:294Returned an error {"targetMethod": "ReconcileKind", "error": "1 error occurred:\n\t* PipelineRun missing object keys for parameters: map[some-param:[key2]]\n\n"} pipelinerun_test.go:1316: error in test pipeline-missing-object-param-keys: expected event "Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing object keys required by Pipeline foo/a-pipeline-with-object-params's parameters: PipelineRun missing object keys for parameters: map[some-param:[key2]]" but got "Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing object keys required by Pipeline foo/a-pipeline-with-object-params's parameters: PipelineRun missing object keys for parameters: map[some-param:[key2]]" instead

That's really weird-- can you try it out on the cluster and see what error message is returned to the user?

sure. Just added. We'll see an error reporting that.

This is the error I refer to when we also add the rest of the error message to the expected wantEvent.

So I guess we need to remove the rest part to pass the unit test @lbernick

(But in the error returned when users miss some keys, we can see the full message without a problem with the information about which keys were missing and for which parameters.)

tekton-robot · 2022-06-15T18:42: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/pipelinerun.go	86.1%	86.2%	0.0

pkg/reconciler/taskrun/validate_resources.go

lbernick · 2022-06-16T19:37:19Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+		permanentError: true,
+		wantEvents: []string{
+			"Normal Started",
+			"Warning Failed PipelineRun foo/pipeline-missing-object-param-keys parameters is missing some object keys",


That's really weird-- can you try it out on the cluster and see what error message is returned to the user?

lbernick · 2022-06-16T19:55:04Z

/lgtm

tekton-robot · 2022-06-16T19:55:58Z

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/pipelinerun.go	86.4%	86.4%	0.0

dibyom · 2022-06-17T20:36:39Z

/test tekton-pipeline-unit-tests

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

Add validation that checks if object param value from PipelineRunSpec misses some keys required for that object param declared in PipelineSpec.

chuangw6 · 2022-06-18T00:33:14Z

/retest

tekton-robot · 2022-06-18T00:35:08Z

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/pipelinerun.go	86.3%	86.3%	0.0

vdemeester · 2022-06-22T12:52:33Z

/kind misc

tekton-robot · 2022-06-22T12:53:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, vdemeester, 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:

~~OWNERS~~ [dibyom,vdemeester]

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

ywluogg · 2022-06-23T21:48:02Z

/lgtm

tekton-robot · 2022-06-23T21:48:10Z

@ywluogg: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

ywluogg · 2022-06-24T14:10:51Z

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 18, 2022

tekton-robot requested review from dibyom and lbernick May 18, 2022 05:46

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 18, 2022

chuangw6 force-pushed the pr-pp-object-key-validation branch from 174a1e4 to fad4020 Compare May 18, 2022 05:59

tekton-robot assigned ywluogg Jun 2, 2022

ywluogg approved these changes Jun 6, 2022

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022

chuangw6 force-pushed the pr-pp-object-key-validation branch from fad4020 to c78367c Compare June 9, 2022 20:32

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022

dibyom approved these changes Jun 13, 2022

View reviewed changes

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

chuangw6 force-pushed the pr-pp-object-key-validation branch from c78367c to 2253bae Compare June 14, 2022 22:00

chuangw6 force-pushed the pr-pp-object-key-validation branch from 2253bae to b87432d Compare June 14, 2022 22:29

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 Jun 14, 2022

chuangw6 force-pushed the pr-pp-object-key-validation branch from b87432d to 13065e2 Compare June 14, 2022 22:41

lbernick reviewed Jun 15, 2022

View reviewed changes

chuangw6 force-pushed the pr-pp-object-key-validation branch from 13065e2 to 02629ca Compare June 15, 2022 18:39

lbernick reviewed Jun 16, 2022

View reviewed changes

chuangw6 force-pushed the pr-pp-object-key-validation branch from 02629ca to db63243 Compare June 16, 2022 19:51

tekton-robot assigned lbernick Jun 16, 2022

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

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022

TEP-0075: Validate object keys, PipelineRunSpec -> PipelineSpec

9b2f96d

Add validation that checks if object param value from PipelineRunSpec misses some keys required for that object param declared in PipelineSpec.

chuangw6 force-pushed the pr-pp-object-key-validation branch from 0c8ac5f to 9b2f96d Compare June 17, 2022 22:17

tekton-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 22, 2022

tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Jun 22, 2022

vdemeester approved these changes Jun 22, 2022

View reviewed changes

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 22, 2022

ywluogg mentioned this pull request Jun 22, 2022

Add ywluogg into Pipeline reviewer roles tektoncd/community#737

Merged

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

tekton-robot merged commit 7e119c5 into tektoncd:main Jun 24, 2022

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-0075] Validate object keys, PipelineRunSpec -> PipelineSpec #4883

[TEP-0075] Validate object keys, PipelineRunSpec -> PipelineSpec #4883

chuangw6 commented May 18, 2022 •

edited

Loading

tekton-robot commented May 18, 2022

tekton-robot commented May 18, 2022

chuangw6 commented May 20, 2022

tekton-robot commented May 20, 2022

ywluogg commented Jun 2, 2022

ywluogg commented Jun 2, 2022

ywluogg commented Jun 2, 2022

ywluogg left a comment

tekton-robot commented Jun 9, 2022

chuangw6 commented Jun 9, 2022

dibyom left a comment

tekton-robot commented Jun 14, 2022

chuangw6 commented Jun 14, 2022 •

edited

Loading

tekton-robot commented Jun 14, 2022

tekton-robot commented Jun 14, 2022

lbernick Jun 15, 2022

chuangw6 Jun 15, 2022 •

edited

Loading

lbernick Jun 16, 2022

chuangw6 Jun 16, 2022 •

edited

Loading

chuangw6 Jun 17, 2022 •

edited

Loading

tekton-robot commented Jun 15, 2022

lbernick Jun 16, 2022

lbernick commented Jun 16, 2022

tekton-robot commented Jun 16, 2022

dibyom commented Jun 17, 2022

chuangw6 commented Jun 18, 2022

tekton-robot commented Jun 18, 2022

vdemeester commented Jun 22, 2022

tekton-robot commented Jun 22, 2022

ywluogg commented Jun 23, 2022

tekton-robot commented Jun 23, 2022

ywluogg commented Jun 24, 2022

[TEP-0075] Validate object keys, PipelineRunSpec -> PipelineSpec #4883

[TEP-0075] Validate object keys, PipelineRunSpec -> PipelineSpec #4883

Conversation

chuangw6 commented May 18, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 18, 2022

tekton-robot commented May 18, 2022

chuangw6 commented May 20, 2022

tekton-robot commented May 20, 2022

ywluogg commented Jun 2, 2022

ywluogg commented Jun 2, 2022

ywluogg commented Jun 2, 2022

ywluogg left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 9, 2022

chuangw6 commented Jun 9, 2022

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 14, 2022

chuangw6 commented Jun 14, 2022 • edited Loading

tekton-robot commented Jun 14, 2022

tekton-robot commented Jun 14, 2022

lbernick Jun 15, 2022

Choose a reason for hiding this comment

chuangw6 Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

lbernick Jun 16, 2022

Choose a reason for hiding this comment

chuangw6 Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

chuangw6 Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Jun 15, 2022

lbernick Jun 16, 2022

Choose a reason for hiding this comment

lbernick commented Jun 16, 2022

tekton-robot commented Jun 16, 2022

dibyom commented Jun 17, 2022

chuangw6 commented Jun 18, 2022

tekton-robot commented Jun 18, 2022

vdemeester commented Jun 22, 2022

tekton-robot commented Jun 22, 2022

ywluogg commented Jun 23, 2022

tekton-robot commented Jun 23, 2022

ywluogg commented Jun 24, 2022

chuangw6 commented May 18, 2022 •

edited

Loading

chuangw6 commented Jun 14, 2022 •

edited

Loading

chuangw6 Jun 15, 2022 •

edited

Loading

chuangw6 Jun 16, 2022 •

edited

Loading

chuangw6 Jun 17, 2022 •

edited

Loading