TEP-0075: More flexible ways to provide values for object param keys #5427

chuangw6 · 2022-09-02T22:19:19Z

Changes

Closes #5270.

Prior to this change, if users want to provide default value for an
object param, they have to provide values for ALL keys, but can't provide
values for a subset of required keys that are declared in properties
section. Same restriction applies to the value provided from run level.

In this PR, we relax the validation for object param keys to allow users
to provide values for an object param in a more flexible way i.e. a subset
of keys are provided in the spec's default, and the rest of the keys are
provided in run level's value.

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

More flexible ways to provide values for object param keys: a subset of keys can be provided from default, and the rest is provided at runtime.

chuangw6 · 2022-09-02T22:20:17Z

/kind feature

chuangw6 · 2022-09-02T22:21:10Z

cc @lbernick @wlynch @chitrangpatel @ywluogg @Yongxuanzhang
please take a look thanks!

tekton-robot · 2022-09-02T22:24: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/apis/pipeline/v1beta1/task_validation.go	98.1%	98.0%	-0.1
pkg/reconciler/taskrun/validate_resources.go	97.7%	98.1%	0.4

afrittoli · 2022-09-06T08:44:01Z

/test check-pr-has-kind-label

tekton-robot · 2022-09-06T08:44:03Z

@afrittoli: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

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

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

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.

afrittoli · 2022-09-06T08:44:36Z

Thank you @chuangw6 - could you please provide release notes for this feature, and check the submitter checklist?

chuangw6 · 2022-09-06T11:32:42Z

Thank you @chuangw6 - could you please provide release notes for this feature, and check the submitter checklist?

Added! Thanks @afrittoli !

tekton-robot · 2022-09-06T11:35:57Z

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/v1/task_validation.go	98.0%	97.8%	-0.1
pkg/apis/pipeline/v1beta1/task_validation.go	98.1%	98.0%	-0.1
pkg/reconciler/taskrun/validate_resources.go	97.7%	98.1%	0.4

chuangw6 · 2022-09-06T12:00:38Z

/retest

lbernick

I think this is actually two separate changes:

if default is provided for all keys, the values from the run will be merged with the defaults
the default does not have to be provided for all keys

Is it clear that we want both of these features?
We might also want to update tep 75 before merging this

lbernick · 2022-09-06T14:12:11Z

pkg/apis/pipeline/v1/task_validation_test.go

@@ -686,27 +686,6 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: fmt.Sprintf("The value type specified for these keys %v is invalid", []string{"key1"}),
 			Paths:   []string{"params.task.properties"},
 		},
-	}, {
-		name: "keys defined in properties are missed in default",


let's keep this test case around but just have it result in a successful validation.

Sounds great! Done. Thanks @lbernick

I think you might not have pushed these changes?

It's under the valid test cases called valid params type explicit and the param name is myobjWithDefaultPartialKeys.

chuangw6 · 2022-09-06T21:21:22Z

~~> * if default is provided for all keys, the values from the run will be merged with the defaults~~

~~Can you elaborate a bit on what merge means in case default is provided for all keys? @lbernick~~

tekton-robot · 2022-09-06T22:19:07Z

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/v1/task_validation.go	98.0%	97.8%	-0.1
pkg/apis/pipeline/v1beta1/task_validation.go	98.1%	98.0%	-0.1
pkg/reconciler/taskrun/validate_resources.go	97.7%	98.1%	0.4

chuangw6 · 2022-09-07T01:23:56Z

the default does not have to be provided for all keys

Not only for default but also the value provided on the taskrun level. Both of them do not have to be provided for all keys. (Prior, the validation enforces that they have to provide values for all keys if they provide value for the object param.)

if default is provided for all keys, the values from the run will be merged with the defaults

This is true. But I'd think this as the implication of that both default and taskrun do not need to provide value for all keys of an object param.

Currently tep75 says "If a value is provided for the param, the default will not be used (i.e. there will be no behavior to merge the default with the provided value, if for example the provided value specified some fields but not others).". If we do not have the merge behaviour, what will happen is that the keys missed at taskrun value will have empty value even though the default provides the value for these keys, which I think probably is a bit unintuitive.

For example, and object param named foo has two keys key1 and key2. The default provides value for both keys (i.e. "default1" for key1, and "default2" for key2), but at taskrun level, only the value of key1 is provided (i.e. "value-runtime").

Without merging behaviour, the resolved object will have "value-runtime" as the value of key1, but empty value for key2.
With merging behaviour, the resolved object will have "value-runtime" as the value of key1, and "default2" as the value of key2.

Is it clear that we want both of these features? We might also want to update tep 75 before merging this

IMHO, the 2nd case might make more sense, but I am open to the discussion here. And I agree that we need to update the tep before merging this.

lbernick · 2022-09-07T13:53:32Z

Thanks for the response @chuangw6!

Let's say we start with not requiring the default to specify all keys. In order for this to be useful, we will either need the merge behavior (so that unspecified defaults can be provided at runtime), or we will need to allow object keys to be optional. Probably both!

We could start with just the merge behavior, but still require the default to specify all keys. There's definitely some examples where this could be useful, e.g. a git object param with a default url like the example you have in this PR.

In the example you provided, without merging behavior, one of the keys would not be set, and the taskrun would fail because we don't support optional keys, correct? Merging behavior would be backwards incompatible in the sense that this would no longer fail, but because we don't support optional keys we don't need to provide a mechanism to "unset" a key at runtime.

I think we might want to start with just the merge behavior to avoid getting into optional vs required keys, WDYT?

chuangw6 · 2022-09-07T21:00:48Z

I think we might want to start with just the merge behavior to avoid getting into optional vs required keys, WDYT?

Starting with merge behaviour to avoid optional v.s. required keys SGTM!

but still require the default to specify all keys.

But it seems to me that default does not have to specify all keys, which is not related to optional/required keys. Currently, the tep proposed that all keys specified in properties are required. Therefore, in the following example, an error will be thrown because we find out that neither taskrun or default provides a value for the required key commit.

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: object-param-test-
spec:
  params:
    - name: gitrepo
      value:
        url: "xyz.com"
  taskSpec:
    params:
      - name: gitrepo
        properties:
          url: {}
          commit: {}
        default:
          url: "default.com"

If the above example provides the value (i.e. "sha123") for key commit at taskrun, this would be valid and resolved object param gitrepo will be like {url:"xyz.com", commit:"sha123"}.

lbernick · 2022-09-08T18:39:01Z

ok -- I'm convinced that this doesn't require optional keys and isn't backwards incompatible (because we were previously requiring the taskrun to provide a value for all keys if it overrode the default)

lbernick · 2022-09-08T18:41:01Z

pkg/apis/pipeline/v1/task_validation_test.go

@@ -686,27 +686,6 @@ func TestTaskSpecValidateError(t *testing.T) {
 			Message: fmt.Sprintf("The value type specified for these keys %v is invalid", []string{"key1"}),
 			Paths:   []string{"params.task.properties"},
 		},
-	}, {
-		name: "keys defined in properties are missed in default",


I think you might not have pushed these changes?

pkg/reconciler/pipelinerun/resources/validate_params_test.go

pkg/reconciler/taskrun/validate_resources_test.go

pkg/apis/pipeline/v1beta1/task_validation_test.go

chuangw6 · 2022-09-08T18:46:57Z

ok -- I'm convinced that this doesn't require optional keys and isn't backwards incompatible (because we were previously requiring the taskrun to provide a value for all keys if it overrode the default)

Correct. Now taskrun can still provide a value for all keys, but they don't have to as long as default provides a value for the keys that are missed at tasrkun.

tekton-robot · 2022-09-09T16:01:17Z

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/v1/task_validation.go	98.0%	97.8%	-0.1
pkg/apis/pipeline/v1beta1/task_validation.go	98.1%	98.0%	-0.1
pkg/reconciler/taskrun/validate_resources.go	97.7%	98.1%	0.4

Related to tektoncd#5270. Prior to this change, if users want to provide default value for an object param, they have to provide values for ALL keys, but can't provide values for a subset of required keys that are declared in `properties` section. Same restriction applies to the `value` provided from run level. In this PR, we relax the validation for object param keys to allow users to provide values for an object param in a more flexible way i.e. a subset of keys are provided in the spec's default, and the rest of the keys are provided in run level's value. Signed-off-by: Chuang Wang <[email protected]>

tekton-robot · 2022-09-09T16:17:47Z

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/v1/task_validation.go	98.0%	97.8%	-0.1
pkg/apis/pipeline/v1beta1/task_validation.go	98.1%	98.0%	-0.1
pkg/reconciler/taskrun/validate_resources.go	97.7%	98.1%	0.4

related to tektoncd/pipeline#5427. Prior to this update, merge behaviour was not allowed between `default` and run level value for object keys. In that proposal and the implementation pr, we allow the merge behaviour to happen to give users more flexible ways to provide values for object param as long as all keys are provided with a value at runtime (combination of the values provided from default and run level value). Signed-off-by: Chuang Wang <[email protected]>

chuangw6 · 2022-09-09T16:45:35Z

I also updated the tep tektoncd/community#817. Please take a look @lbernick. Thanks!

pkg/reconciler/taskrun/validate_resources_test.go

related to tektoncd/pipeline#5427. Prior to this update, merge behaviour was not allowed between `default` and run level value for object keys. In that proposal and the implementation pr, we allow the merge behaviour to happen to give users more flexible ways to provide values for object param as long as all keys are provided with a value at runtime (combination of the values provided from default and run level value). Signed-off-by: Chuang Wang <[email protected]>

tekton-robot · 2022-09-12T08:43:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, vdemeester

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~~ [lbernick,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-09-12T14:43:14Z

pkg/apis/pipeline/v1/task_validation.go

-}
-
-// ValidateObjectKeys validates if object keys defined in properties are all provided in its value provider iff the provider is not nil.
-func ValidateObjectKeys(properties map[string]PropertySpec, propertiesProvider *ParamValue) (errs *apis.FieldError) {


Ah can you leave this function as is? I'm planning to use it in Chains. I think initially you are making this function public because I need to do keys comparisons in Chains, is it?

Oh NVM because you are not using this function anymore in Pipeline, I will create this one in Chains then. Please ignore the previous commnent

ywluogg

/lgtm

chuangw6 · 2022-09-12T15:38:26Z

/test pull-tekton-pipeline-integration-tests

chuangw6 · 2022-09-12T16:13:27Z

Tep 75 was updated to reflect the change as well. tektoncd/community#817

related to tektoncd/pipeline#5427. Prior to this update, merge behaviour was not allowed between `default` and run level value for object keys. In that proposal and the implementation pr, we allow the merge behaviour to happen to give users more flexible ways to provide values for object param as long as all keys are provided with a value at runtime (combination of the values provided from default and run level value). Signed-off-by: Chuang Wang <[email protected]>

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 2, 2022

tekton-robot requested review from jerop and pritidesai September 2, 2022 22:19

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 2, 2022

chuangw6 changed the title ~~TEP-0075: Relax object param key validation~~ TEP-0075: More flexible ways to provide values for object param keys Sep 2, 2022

lbernick self-assigned this Sep 5, 2022

chuangw6 force-pushed the relax-object-param-validation branch from 189bb11 to d850ab8 Compare September 6, 2022 11:30

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 Sep 6, 2022

lbernick reviewed Sep 6, 2022

View reviewed changes

chuangw6 force-pushed the relax-object-param-validation branch from d850ab8 to 3b7ba03 Compare September 6, 2022 22:14

lbernick mentioned this pull request Sep 8, 2022

Propagating object params #5371

Merged

7 tasks

lbernick reviewed Sep 8, 2022

View reviewed changes

chuangw6 force-pushed the relax-object-param-validation branch from 3b7ba03 to 868bd21 Compare September 9, 2022 15:56

chuangw6 force-pushed the relax-object-param-validation branch from 868bd21 to e83ade5 Compare September 9, 2022 16:09

chuangw6 force-pushed the relax-object-param-validation branch from e83ade5 to 8e17261 Compare September 9, 2022 16:12

chuangw6 requested review from lbernick and removed request for pritidesai and jerop September 9, 2022 16:25

chuangw6 mentioned this pull request Sep 9, 2022

TEP-0075: More flexible ways to provide values for object param tektoncd/community#817

Merged

lbernick approved these changes Sep 9, 2022

View reviewed changes

pkg/reconciler/taskrun/validate_resources_test.go Show resolved Hide resolved

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

vdemeester approved these changes Sep 12, 2022

View reviewed changes

ywluogg reviewed Sep 12, 2022

View reviewed changes

tekton-robot assigned ywluogg Sep 12, 2022

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

tekton-robot merged commit 80fc393 into tektoncd:main Sep 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TEP-0075: More flexible ways to provide values for object param keys #5427

TEP-0075: More flexible ways to provide values for object param keys #5427

chuangw6 commented Sep 2, 2022 •

edited

Loading

chuangw6 commented Sep 2, 2022

chuangw6 commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

afrittoli commented Sep 6, 2022

chuangw6 commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

chuangw6 commented Sep 6, 2022

lbernick left a comment

lbernick Sep 6, 2022

chuangw6 Sep 6, 2022

lbernick Sep 8, 2022

chuangw6 Sep 9, 2022

chuangw6 commented Sep 6, 2022 •

edited

Loading

tekton-robot commented Sep 6, 2022

chuangw6 commented Sep 7, 2022 •

edited

Loading

lbernick commented Sep 7, 2022

chuangw6 commented Sep 7, 2022 •

edited

Loading

lbernick commented Sep 8, 2022

lbernick Sep 8, 2022

chuangw6 commented Sep 8, 2022 •

edited

Loading

tekton-robot commented Sep 9, 2022

tekton-robot commented Sep 9, 2022

chuangw6 commented Sep 9, 2022

tekton-robot commented Sep 12, 2022

ywluogg Sep 12, 2022

ywluogg Sep 12, 2022 •

edited

Loading

ywluogg left a comment

chuangw6 commented Sep 12, 2022

chuangw6 commented Sep 12, 2022

TEP-0075: More flexible ways to provide values for object param keys #5427

TEP-0075: More flexible ways to provide values for object param keys #5427

Conversation

chuangw6 commented Sep 2, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

chuangw6 commented Sep 2, 2022

chuangw6 commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

afrittoli commented Sep 6, 2022

chuangw6 commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

chuangw6 commented Sep 6, 2022

lbernick left a comment

Choose a reason for hiding this comment

lbernick Sep 6, 2022

Choose a reason for hiding this comment

chuangw6 Sep 6, 2022

Choose a reason for hiding this comment

lbernick Sep 8, 2022

Choose a reason for hiding this comment

chuangw6 Sep 9, 2022

Choose a reason for hiding this comment

chuangw6 commented Sep 6, 2022 • edited Loading

tekton-robot commented Sep 6, 2022

chuangw6 commented Sep 7, 2022 • edited Loading

lbernick commented Sep 7, 2022

chuangw6 commented Sep 7, 2022 • edited Loading

lbernick commented Sep 8, 2022

lbernick Sep 8, 2022

Choose a reason for hiding this comment

chuangw6 commented Sep 8, 2022 • edited Loading

tekton-robot commented Sep 9, 2022

tekton-robot commented Sep 9, 2022

chuangw6 commented Sep 9, 2022

tekton-robot commented Sep 12, 2022

ywluogg Sep 12, 2022

Choose a reason for hiding this comment

ywluogg Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

ywluogg left a comment

Choose a reason for hiding this comment

chuangw6 commented Sep 12, 2022

chuangw6 commented Sep 12, 2022

chuangw6 commented Sep 2, 2022 •

edited

Loading

chuangw6 commented Sep 6, 2022 •

edited

Loading

chuangw6 commented Sep 7, 2022 •

edited

Loading

chuangw6 commented Sep 7, 2022 •

edited

Loading

chuangw6 commented Sep 8, 2022 •

edited

Loading

ywluogg Sep 12, 2022 •

edited

Loading