Modify spec params to avoid confusion between resourcetemplates and triggertemplate params #589

savitaashture · 2020-05-29T15:25:49Z

Changes

PR helps to avoid confusion from TriggerTemplate params to resourceTemplate params
by keeping params of spec as it is and using $(tt.params.) for the substitution.

So, this way we no need to handle the backward compatibility as per the discussion over the comments

Fixes #508

Tried to do tag configurable, given the way ObjectDefaulter and SetDefaults work, neither could accomplish the switch properly. ObjectDefaulter could have done it, but the way the tests are configured do not cause the Defaults to be executed since the objects aren't actually decoded.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

The substitution for params modified from $(params.gitrevision) to $(tt.params.gitrevision) .
And the the examples

apiVersion: triggers.tekton.dev/v1alpha1
kind: TriggerTemplate
metadata:
  name: github-template
spec:
  params:
    - name: gitrevision
    - name: gitrepositoryurl
  resourcetemplates:
    - apiVersion: tekton.dev/v1alpha1
      kind: TaskRun
      metadata:
        generateName: github-run-
      spec:
        taskSpec:
          inputs:
            resources:
              - name: source
                type: git
          steps:
            - image: ubuntu
              script: |
                #! /bin/bash
                ls -al $(inputs.resources.source.path)
        inputs:
          resources:
            - name: source
              resourceSpec:
                type: git
                params:
                  - name: revision
                    value: $(tt.params.gitrevision)
                  - name: url
                    value: $(tt.params.gitrepositoryurl)

For a release or two $(params) supported and later it will be deprecated.
Created issue to track the same

cc @dibyom

tekton-robot · 2020-05-29T15:27:45Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/trigger_template_validation.go	97.2%	97.5%	0.3

dibyom · 2020-06-01T20:06:15Z

Thanks for opening this @savitaashture I won't get to this today but will put in on my list for tomorrow!

dibyom · 2020-06-03T14:36:02Z

params are now deprecated and will be removed in future but for now TriggerTemplate spec can support param as shown below

Might be worth rewording as - The params field in TriggerTemplates has been deperecated in favor of the ttparams field. And the the examples

dibyom

Thanks @savitaashture .Code looks fine to me. I'll try this out on my env. Given that this is a pretty far reaching API change, let's discuss it once in next week's WG before merging?

dibyom · 2020-06-02T21:06:24Z

pkg/apis/triggers/v1alpha1/trigger_template_defaults.go

-func (tt *TriggerTemplate) SetDefaults(ctx context.Context) {}
+func (tt *TriggerTemplate) SetDefaults(ctx context.Context) {
+	// This is a temporary logic to have a backward compatibility with the original params.
+	// TODO(savitaashture): remove below logic once after the complete deprecation of Spec.DeprecatedParams


we should open an issue to remove this and link to that issue here instead!

Sure i will open and update in the code comments

Yes +1 to discuss before merging

Created issue to track the removal of deprecated tag params

dibyom · 2020-06-03T14:34:18Z

pkg/apis/triggers/v1alpha1/trigger_template_validation.go

 			if _, ok := declaredParamNames[templateParamName]; !ok {
+				// This logic is to get the tag and display in error dynamically for both ttparams and params.
+				// TODO(savitaashture): remove params once after the complete deprecation of spec.DeprecatedParams
+				var tag string


Question: Until we start using only $(ttparams) for replacement, we cannot support #525 right?

Yes because till that time we will have validation for params

dibyom · 2020-06-03T14:37:45Z

/approve
/hold

tekton-robot · 2020-06-03T14:37:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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]

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

tekton-robot · 2020-06-12T17:05:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/trigger_template_validation.go	97.2%	97.5%	0.3

bobcatfish · 2020-06-17T16:19:11Z

Drive by comment - instead of changing the name of the params field, you could change how the replacement works, e.g. use $(tt.params.foo) when using the field, but still call the section params in the trigger template spec

whaddaya think (ignore me if this has been discussed already)

savitaashture · 2020-06-17T16:50:33Z

Drive by comment - instead of changing the name of the params field, you could change how the replacement works, e.g. use $(tt.params.foo) when using the field, but still call the section params in the trigger template spec

whaddaya think (ignore me if this has been discussed already)

Hi @bobcatfish Its good point
Right now I don't remember that whether we discussed the point which you mentioned.
But i just want to know any reason behind this $(tt.params.foo) suggestion
Or is it because consistency with params

bobcatfish · 2020-06-17T19:23:50Z

But i just want to know any reason behind this $(tt.params.foo) suggestion
Or is it because consistency with params

Just consistency! Across tekton we have multiple spec.params - it seems like the right name for the field. The problem is the ambiguity in the usage, feels weird to solve that by changing the name of the field.

Mostly a subjective opinion tho!

dibyom · 2020-06-17T20:43:15Z

I like the $(tt.params) idea. I think we discussed this though there weren't any strong opinions one way or another.
I agree with the consistency issue and that renaming the field feels a bit weird. Calling the field params but using it as tt.params is also weird but less so in my opinion 👼

How do you feel about keeping the params field and using tt.params for substitution? Also if we go ahead with this: I think we should support replacements with both $(params) and $(tt.params) for a release before we drop support for $(params)?

savitaashture · 2020-06-18T15:32:36Z

I like the $(tt.params) idea. I think we discussed this though there weren't any strong opinions one way or another.
I agree with the consistency issue and that renaming the field feels a bit weird. Calling the field params but using it as tt.params is also weird but less so in my opinion

How do you feel about keeping the params field and using tt.params for substitution? Also if we go ahead with this: I think we should support replacements with both $(params) and $(tt.params) for a release before we drop support for $(params)?

@dibyom yes i think we need to keep $(params) and $(tt.params) for a release before we drop support for $(params)

Let me understand clearly one of your point here
How do you feel about keeping the params field and using tt.params for substitution?
Do you mean something like below

spec:
  params:
    - name: gitrevision
    - name: gitrepositoryurl
  resourcetemplates:
    - apiVersion: tekton.dev/v1alpha1
      kind: TaskRun
      metadata:
        generateName: github-run-
      spec:
        taskSpec:
          inputs:
            resources:
              - name: source
                type: git
          steps:
            - image: ubuntu
              script: |
                #! /bin/bash
                ls -al $(inputs.resources.source.path)
        inputs:
          resources:
            - name: source
              resourceSpec:
                type: git
                params:
                  - name: revision
                    value: $(tt.params.gitrevision)
                  - name: url
                    value: $(tt.params.gitrepositoryurl)

If thats the thing i feel it creates bit confusion 🤔

OR

This way

spec:
  tt:
    params:
      - name: gitrevision
      - name: gitrepositoryurl
  resourcetemplates:
    - apiVersion: tekton.dev/v1alpha1
      kind: TaskRun
      metadata:
        generateName: github-run-
      spec:
        taskSpec:
          inputs:
            resources:
              - name: source
                type: git
          steps:
            - image: ubuntu
              script: |
                #! /bin/bash
                ls -al $(inputs.resources.source.path)
        inputs:
          resources:
            - name: source
              resourceSpec:
                type: git
                params:
                  - name: revision
                    value: $(tt.params.gitrevision)
                  - name: url
                    value: $(tt.params.gitrepositoryurl)

dibyom · 2020-06-18T16:09:15Z

Yeah, the first one. I agree it can be a bit confusing 😄 On the other hand calling the field ttparams when everywhere else (e.g. TriggerBindings, Pipelines etc.) we call it params is also a bit inconsistent.

savitaashture · 2020-06-18T16:15:29Z

Yeah, the first one. I agree it can be a bit confusing On the other hand calling the field ttparams when everywhere else (e.g. TriggerBindings, Pipelines etc.) we call it params is also a bit inconsistent.

Ya even i am thinking of confusing part.
Otherwise + point is we no need to do backward compatibility if we keep field params as it is and use $(tt.params) everywhere

savitaashture · 2020-06-20T21:01:59Z

@dibyom Modified as per the discussion

Changes:
kept $(params) field and used $(tt.params) for the substitution

savitaashture · 2020-06-20T21:17:52Z

/retest

dibyom · 2020-06-22T15:46:55Z

pkg/template/resource.go

@@ -111,7 +111,7 @@ func ApplyParamsToResourceTemplate(params []triggersv1.Param, rt json.RawMessage
 // param value substituted for all matching param variables in the template
 func applyParamToResourceTemplate(param triggersv1.Param, rt json.RawMessage) json.RawMessage {
 	// Assume the param is valid
-	paramVariable := fmt.Sprintf("$(params.%s)", param.Name)
+	paramVariable := fmt.Sprintf("$(tt.params.%s)", param.Name)


Are we just supporting $(tt.params) for now. Shouldn't we support both $(params) or $(tt.params) for a release or two?

Updated changes
Please do have a look
Thank you

tekton-robot · 2020-06-22T18:42:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/trigger_template_validation.go	97.2%	97.5%	0.3

tekton-robot · 2020-06-22T18:49:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/trigger_template_validation.go	97.2%	97.5%	0.3

dibyom · 2020-06-23T02:05:33Z

pkg/template/resource.go

 	// Assume the param is valid
-	paramVariable := fmt.Sprintf("$(params.%s)", param.Name)
+	if len(templateParams) == 0 {
+		paramVariable = fmt.Sprintf("$(tt.params.%s)", param.Name)


Is there a reason we cannot attempt to replace with both $(params) as well as $(tt.params)?

The reason is we will not be getting the information on user provided ResourceTemplate params whether its $(params) or $(tt.params)

So currently with the help of regular expression able to distinguish whether its $(params) or $(tt.params).

I don't think this works if a user mixes both params and tt.params in the same file. It should be safe to do a double pass - e.g.

for _, tag := range []string{ "params", "tt.params"} { paramVariable := fmt.Sprintf("$(%s.%s)", tag, param.Name) rt = bytes.Replace(rt, []byte(paramVariable), []byte(paramValue), -1) }

khrm · 2020-06-23T16:29:34Z

docs/triggertemplates.md

          - name: url
-            value: $(params.gitrepositoryurl)
+            value: $(tt.params.gitrepositoryurl)
 ```

 `TriggerTemplates` currently support the following [Tekton Pipelines](https://github.com/tektoncd/pipelines) resources:


We need to update https://github.com/tektoncd/triggers/blob/ff20c8592a1b419ac70b2f46987ded3658bfeada/docs/triggertemplates.md#parameters

Updated doc 👍
Thank you

tekton-robot · 2020-06-23T18:11:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/trigger_template_validation.go	97.2%	97.5%	0.3

savitaashture · 2020-06-23T18:26:14Z

/test pull-tekton-triggers-integration-tests

dibyom · 2020-06-23T20:02:55Z

/test pull-tekton-triggers-integration-tests

wlynch

Almost there! No particular preference on tt.params (only alternative I can think of is template.params, but I don't care that much).

Few minor comments, otherwise SGTM

wlynch · 2020-06-23T20:27:03Z

pkg/apis/triggers/v1alpha1/trigger_template_validation.go

 		templateParams := paramsRegexp.FindAllSubmatch(template.RawExtension.Raw, -1)
 		for _, templateParam := range templateParams {
-			templateParamName := string(templateParam[1])
+			templateParamName := string(templateParam[2])


I realize this is following what was set before, but the direct array access is at risk for nil panics. I think we should do 2 things:

Have a length check prior to accessing templateParam[2].

Create a more exhaustive set of regexp checks instead of the more e2e tests we see here. (mainly to give us more confidence that the regexp is matching to what we expect, even in unexpected cases).

Actually

templateParams := paramsRegexp.FindAllSubmatch(template.RawExtension.Raw, -1)

templateParams length will be 0 in any unexpected cases because of Regular Expression

regexp.MustCompile(`\$$(params|tt.params).([_a-zA-Z][_a-zA-Z0-9.-]*)$`)

If user provides any such values like below

1. params1.foo 2. tt1.params.foo 3. tt.params1.foo 4. tt.foo 5. .foo

the templateParams itself is 0 and because of that it never enters into for loop

But yes need to add more test cases to ensures the regex will not break.

Added test cases to cover few more scenario

wlynch · 2020-06-23T20:28:49Z

pkg/sink/sink_test.go

@@ -153,18 +153,18 @@ func TestHandleEvent(t *testing.T) {
 			Kind:       "PipelineResource",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      "$(params.name)",
+			Name:      "$(tt.params.name)",


We should probably leave some of these tests using the old params syntax to ensure backwards compatibility, since this is the first place where we are actually using the value outside of validation.

+1 to having some tests with the old $(params)

wlynch · 2020-06-23T20:41:07Z

pkg/template/resource.go

 	// Assume the param is valid
-	paramVariable := fmt.Sprintf("$(params.%s)", param.Name)
+	if len(templateParams) == 0 {
+		paramVariable = fmt.Sprintf("$(tt.params.%s)", param.Name)


I don't think this works if a user mixes both params and tt.params in the same file. It should be safe to do a double pass - e.g.

for _, tag := range []string{ "params", "tt.params"} { paramVariable := fmt.Sprintf("$(%s.%s)", tag, param.Name) rt = bytes.Replace(rt, []byte(paramVariable), []byte(paramValue), -1) }

tekton-robot · 2020-06-24T10:19:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/trigger_template_validation.go	97.2%	97.5%	0.3

…riggertemplate params

tekton-robot · 2020-06-24T10:27:39Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/triggers/v1alpha1/trigger_template_validation.go	97.2%	97.5%	0.3

dibyom

/lgtm

tekton-robot requested review from dlorenc and wlynch May 29, 2020 15:25

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

dibyom reviewed Jun 3, 2020

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2020

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

savitaashture mentioned this pull request Jun 12, 2020

Remove deprecated params from TriggerTemplate spec #606

Closed

savitaashture force-pushed the confusingparams branch from 470b7f8 to 8c99a0a Compare June 12, 2020 17:03

savitaashture force-pushed the confusingparams branch from 8c99a0a to 2bbdf49 Compare June 20, 2020 20:59

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

savitaashture changed the title ~~Modify TriggerTemplate spec params to avoid confusion from resourcetemplates params~~ Modify spec params to avoid confusion between resourcetemplates and triggertemplate params Jun 20, 2020

dibyom reviewed Jun 22, 2020

View reviewed changes

savitaashture force-pushed the confusingparams branch from 2bbdf49 to 52b1dbb Compare June 22, 2020 18:40

tekton-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 22, 2020

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 22, 2020

savitaashture force-pushed the confusingparams branch from 52b1dbb to ff20c85 Compare June 22, 2020 18:48

dibyom reviewed Jun 23, 2020

View reviewed changes

dibyom removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2020

khrm reviewed Jun 23, 2020

View reviewed changes

savitaashture force-pushed the confusingparams branch from ff20c85 to 6120b05 Compare June 23, 2020 18:09

wlynch requested changes Jun 23, 2020

View reviewed changes

dibyom mentioned this pull request Jun 23, 2020

Release Triggers v0.6.0 #632

Closed

savitaashture force-pushed the confusingparams branch from 6120b05 to c648333 Compare June 24, 2020 10:18

Modify spec params to avoid confusion between resourcetemplates and t…

2e7a780

…riggertemplate params

savitaashture force-pushed the confusingparams branch from c648333 to 2e7a780 Compare June 24, 2020 10:25

dibyom reviewed Jun 24, 2020

View reviewed changes

tekton-robot assigned dibyom Jun 24, 2020

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

tekton-robot merged commit d11c7bd into tektoncd:master Jun 24, 2020

savitaashture mentioned this pull request Jul 28, 2020

Remove deprecated $(params) to avoid confusion between resourcetemplates and triggertemplate params #690

Merged

3 tasks

dibyom mentioned this pull request Aug 31, 2021

Trigger fails to work as documented on kind cluster #1184

Closed

Modify spec params to avoid confusion between resourcetemplates and triggertemplate params #589

Modify spec params to avoid confusion between resourcetemplates and triggertemplate params #589

Conversation

savitaashture commented May 29, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 29, 2020

dibyom commented Jun 1, 2020

dibyom commented Jun 3, 2020

dibyom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savitaashture Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

savitaashture Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Jun 3, 2020

tekton-robot commented Jun 3, 2020

tekton-robot commented Jun 12, 2020

bobcatfish commented Jun 17, 2020

savitaashture commented Jun 17, 2020

bobcatfish commented Jun 17, 2020

dibyom commented Jun 17, 2020

savitaashture commented Jun 18, 2020

dibyom commented Jun 18, 2020

savitaashture commented Jun 18, 2020

savitaashture commented Jun 20, 2020 • edited Loading

savitaashture commented Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jun 22, 2020

tekton-robot commented Jun 22, 2020

Choose a reason for hiding this comment

savitaashture Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savitaashture Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Jun 23, 2020

savitaashture commented Jun 23, 2020

dibyom commented Jun 23, 2020

wlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savitaashture Jun 24, 2020 • edited Loading

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 Jun 24, 2020

tekton-robot commented Jun 24, 2020

dibyom left a comment

Choose a reason for hiding this comment

savitaashture commented May 29, 2020 •

edited

Loading

savitaashture Jun 8, 2020 •

edited

Loading

savitaashture Jun 12, 2020 •

edited

Loading

savitaashture commented Jun 20, 2020 •

edited

Loading

savitaashture Jun 23, 2020 •

edited

Loading

savitaashture Jun 23, 2020 •

edited

Loading

savitaashture Jun 24, 2020 •

edited

Loading