Add SetDefaults for pipelineSpec in reconciler #5176

Yongxuanzhang · 2022-07-20T15:16:18Z

Changes

This commit adds SetDefaults for pipelineSpec in reconciler
after it is fetched from cluster. This is to avoid failing the resources that
are not mutated and add default values.

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

NONE

tekton-robot · 2022-07-20T15:16:20Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Yongxuanzhang · 2022-07-20T15:16:31Z

/kind bug

Yongxuanzhang · 2022-07-20T15:40:36Z

#5175

Yongxuanzhang · 2022-07-20T15:43:14Z

@jerop @dibyom

tekton-robot · 2022-07-20T15: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/apis/pipeline/v1/task_validation.go	97.5%	97.5%	0.0

abayer · 2022-07-20T16:08:59Z

/retest

afrittoli · 2022-07-20T16:13:55Z

pkg/apis/pipeline/v1/task_validation.go

@@ -274,6 +274,10 @@ func ValidateParameterTypes(ctx context.Context, params []ParamSpec) (errs *apis

 // ValidateType checks that the type of a ParamSpec is allowed and its default value matches that type
 func (p ParamSpec) ValidateType() *apis.FieldError {
+	// For resources not call SetDefaults(), empty type should be valid.


NIT - suggested comment

A resource may have been created before type was available in paramSpec and thus have no type set

couldn't we call SetDefaults() before calling validation instead of just skipping validation?

couldn't we call SetDefaults() before calling validation instead of just skipping validation?

yeah I would prefer this way! I'm testing if that can work

(or at least in this function, we should assume no type == defaultType)

tekton-robot · 2022-07-20T16:28:43Z

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	97.5%	97.5%	0.0
pkg/apis/pipeline/v1beta1/task_validation.go	97.7%	97.7%	0.0

abayer · 2022-07-20T16:52:27Z

/retest

ywluogg · 2022-07-20T17:13:23Z

pkg/apis/pipeline/v1/task_validation.go

@@ -274,6 +274,10 @@ func ValidateParameterTypes(ctx context.Context, params []ParamSpec) (errs *apis

 // ValidateType checks that the type of a ParamSpec is allowed and its default value matches that type
 func (p ParamSpec) ValidateType() *apis.FieldError {
+	// For resources not call SetDefaults(), empty type should be valid.
+	if p.Type == "" {


Can you add a unit test in pipeline reconciler that the param type is null, to make sure this wouldn't introduce any new bugs? Thanks

chuangw6 · 2022-07-20T17:22:17Z

pkg/apis/pipeline/v1beta1/task_validation_test.go

+			if err := ts.Validate(context.Background()); err != nil {
+				t.Errorf("TaskSpec.Validate() = %v", err)
+			}
+		})


In the test, can we compare the returned v.s. expected error like this example rather than just check if err is nil?

Yongxuanzhang · 2022-07-20T17:59:35Z

/hold

abayer · 2022-07-20T19:32:31Z

It's worth mentioning that when I deleted a problem Pipeline and recreated it, with any of kubectl replace, kubectl apply, or kubectl create, params without types specified still had no types specified. This is with Pipeline v0.37.2. So if the current behavior, before this PR, is supposed to be defaulting newly created Pipelines and Tasks to have default types on their params, well, that's not working either.

chuangw6 · 2022-07-20T19:55:22Z

It's worth mentioning that when I deleted a problem Pipeline and recreated it, with any of kubectl replace, kubectl apply, or kubectl create, params without types specified still had no types specified. This is with Pipeline v0.37.2. So if the current behavior, before this PR, is supposed to be defaulting newly created Pipelines and Tasks to have default types on their params, well, that's not working either.

Hi @abayer,
I tried the following yaml (the type of param arg and mystring is not specified) with v0.37.2 and it works fine. Can you also try that? If this example works, let's find what are the missing/different part in the problem pipeline.

kind: Task
apiVersion: tekton.dev/v1beta1
metadata:
  name: test-task
spec:
  params:
    - name: arg
  steps:
    - name: echo
      image: bash:latest
      args: ["echo", "$(params.arg)"]
---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: test-pipeline
spec:
  params:
    - name: mystring
  tasks:
    - name: echo-test
      params:
        - name: arg
          value: $(params.mystring)
      taskRef:
        name: test-task
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: test-pipeline-run
spec:
  params:
    - name: mystring
      value: abc.com
  pipelineRef:
    name: test-pipeline

abayer · 2022-07-20T20:18:11Z

Ok, turns out the problem was incredibly stupid and specific - in the default namespace on the dogfooding cluster, we had webhooks.knative.dev/exclude: "true" in the labels, so it was skipping defaulting entirely. 🤦

ywluogg · 2022-07-20T20:28:16Z

Ok, turns out the problem was incredibly stupid and specific - in the default namespace on the dogfooding cluster, we had webhooks.knative.dev/exclude: "true" in the labels, so it was skipping defaulting entirely. 🤦

🤦🤦🤦

tekton-robot · 2022-07-21T17:32: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	85.8%	85.9%	0.0
pkg/reconciler/taskrun/taskrun.go	80.3%	80.4%	0.1

tekton-robot · 2022-07-21T17:38: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	85.8%	85.9%	0.0
pkg/reconciler/taskrun/taskrun.go	80.3%	80.4%	0.1

tekton-robot · 2022-07-21T17:46:55Z

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	85.8%	85.9%	0.0
pkg/reconciler/taskrun/taskrun.go	80.3%	80.4%	0.1

tekton-robot · 2022-07-21T19:00:40Z

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	85.8%	85.9%	0.0
pkg/reconciler/taskrun/taskrun.go	80.3%	80.4%	0.1

lbernick

I'm a bit confused about what this PR is doing given the original PR message and the conversation that's happened. It looks like this sets defaults in the reconciler to ensure that defaults are applied to resources that were created before these code changes were applied to the server. Is this still an issue with params without types, disregarding the annotations that were mistakenly kept around on our dogfooding cluster?

lbernick · 2022-07-21T19:17:08Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -565,6 +565,7 @@ spec:
  serviceAccountName: test-sa
  taskRef:
    name: unit-test-task
+    kind: Task


What's the reason to add kind: Task to all of these taskRefs?

setdefaults will add kind for task as well, if not adding them the current tests will fail

tekton-robot · 2022-07-21T19:22: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/reconciler/pipelinerun/pipelinerun.go	85.8%	85.9%	0.0
pkg/reconciler/taskrun/taskrun.go	80.3%	80.4%	0.1

tekton-robot · 2022-07-21T20:01:23Z

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	85.8%	85.9%	0.0

Yongxuanzhang · 2022-07-21T20:09:32Z

I'm a bit confused about what this PR is doing given the original PR message and the conversation that's happened. It looks like this sets defaults in the reconciler to ensure that defaults are applied to resources that were created before these code changes were applied to the server.

Yes, that's the goal

Is this still an issue with params without types, disregarding the annotations that were mistakenly kept around on our dogfooding cluster?

There are some other discussions in this issue #5175. And for taskrun when we get the taskspec, it will call taskSpec.SetDefaults(ctx),

pipeline/pkg/reconciler/taskrun/resources/taskspec.go

Lines 46 to 55 in 10a5ae6

    
           case taskRun.Spec.TaskRef != nil && taskRun.Spec.TaskRef.Name != "": 
        
           	// Get related task for taskrun 
        
           	t, err := getTask(ctx, taskRun.Spec.TaskRef.Name) 
        
           	if err != nil { 
        
           		return nil, nil, fmt.Errorf("error when listing tasks for taskRun %s: %w", taskRun.Name, err) 
        
           	} 
        
           	taskMeta = t.TaskMetadata() 
        
           	taskSpec = t.TaskSpec() 
        
           	taskSpec.SetDefaults(ctx) 
        
           case taskRun.Spec.TaskSpec != nil:

but for pipelinerun we don't have it. It may make sense to add for pipelinerun as well

pipeline/pkg/reconciler/pipelinerun/pipelinespec/pipelinespec.go

Lines 40 to 48 in 10a5ae6

    
           case pipelineRun.Spec.PipelineRef != nil && pipelineRun.Spec.PipelineRef.Name != "": 
        
           	// Get related pipeline for pipelinerun 
        
           	t, err := getPipeline(ctx, pipelineRun.Spec.PipelineRef.Name) 
        
           	if err != nil { 
        
           		return nil, nil, fmt.Errorf("error when listing pipelines for pipelineRun %s: %w", pipelineRun.Name, err) 
        
           	} 
        
           	pipelineMeta = t.PipelineMetadata() 
        
           	pipelineSpec = t.PipelineSpec() 
        
           case pipelineRun.Spec.PipelineSpec != nil:

This commit adds SetDefaults for pipelineSpec in reconciler after they are fetched. This is to avoid failing the resources that are not mutated to add default values.

Yongxuanzhang · 2022-07-21T20:37:53Z

/retest

vdemeester · 2022-07-22T09:18:47Z

but for pipelinerun we don't have it. It may make sense to add for pipelinerun as well

indeed that's probably a mistake, it should be there for pipelinerun as well.

tekton-robot · 2022-07-22T14:46:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick

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]

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

Yongxuanzhang · 2022-07-25T15:40:42Z

/hold cancel

ywluogg · 2022-07-26T15:29:28Z

/lgtm

Cherry-picked from tektoncd#5176 This commit adds SetDefaults for pipelineSpec in reconciler after they are fetched. This is to avoid failing the resources that are not mutated to add default values. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

Cherry-picked from #5176 This commit adds SetDefaults for pipelineSpec in reconciler after they are fetched. This is to avoid failing the resources that are not mutated to add default values. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

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-none Denotes a PR that doesnt merit a release note. labels Jul 20, 2022

tekton-robot requested review from imjasonh and pritidesai July 20, 2022 15:16

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

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 20, 2022

Yongxuanzhang marked this pull request as ready for review July 20, 2022 15:40

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 Jul 20, 2022

tekton-robot requested a review from abayer July 20, 2022 15:40

Yongxuanzhang closed this Jul 20, 2022

Yongxuanzhang reopened this Jul 20, 2022

afrittoli added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Jul 20, 2022

afrittoli reviewed Jul 20, 2022

View reviewed changes

Yongxuanzhang force-pushed the fix-param-type-validation branch from 69c4079 to 57025e4 Compare July 20, 2022 16:24

ywluogg reviewed Jul 20, 2022

View reviewed changes

chuangw6 reviewed Jul 20, 2022

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 Jul 20, 2022

ywluogg mentioned this pull request Jul 21, 2022

[TEP-0075]Add more setdefaults features for taskresults #5142

Merged

7 tasks

Yongxuanzhang force-pushed the fix-param-type-validation branch from 2b46c0b to cc495aa Compare July 21, 2022 17:42

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

lbernick reviewed Jul 21, 2022

View reviewed changes

Yongxuanzhang force-pushed the fix-param-type-validation branch from b7d17a0 to 3333072 Compare July 21, 2022 20:15

Yongxuanzhang changed the title ~~Param without types should not fail validation~~ Add SetDefaults for pipelineSpec in reconciler Jul 21, 2022

Add SetDefaults for pipelineSpec in reconciler

8f0ba47

This commit adds SetDefaults for pipelineSpec in reconciler after they are fetched. This is to avoid failing the resources that are not mutated to add default values.

Yongxuanzhang force-pushed the fix-param-type-validation branch from 3333072 to 8f0ba47 Compare July 21, 2022 20:24

lbernick approved these changes Jul 22, 2022

View reviewed changes

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

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2022

tekton-robot assigned ywluogg Jul 26, 2022

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

tekton-robot merged commit 475a562 into tektoncd:main Jul 26, 2022

abayer mentioned this pull request Jul 26, 2022

[cherry-pick: v0.38] Add SetDefaults for pipelineSpec in reconciler #5217

Merged

7 tasks

Add SetDefaults for pipelineSpec in reconciler #5176

Add SetDefaults for pipelineSpec in reconciler #5176

Conversation

Yongxuanzhang commented Jul 20, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 20, 2022

Yongxuanzhang commented Jul 20, 2022

Yongxuanzhang commented Jul 20, 2022

Yongxuanzhang commented Jul 20, 2022 • edited Loading

tekton-robot commented Jul 20, 2022

abayer commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jul 20, 2022

abayer commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang commented Jul 20, 2022

abayer commented Jul 20, 2022

chuangw6 commented Jul 20, 2022

abayer commented Jul 20, 2022

ywluogg commented Jul 20, 2022

tekton-robot commented Jul 21, 2022

tekton-robot commented Jul 21, 2022

tekton-robot commented Jul 21, 2022

tekton-robot commented Jul 21, 2022

lbernick left a 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 Jul 21, 2022

tekton-robot commented Jul 21, 2022

Yongxuanzhang commented Jul 21, 2022 • edited Loading

Yongxuanzhang commented Jul 21, 2022

vdemeester commented Jul 22, 2022

tekton-robot commented Jul 22, 2022

Yongxuanzhang commented Jul 25, 2022

ywluogg commented Jul 26, 2022

Yongxuanzhang commented Jul 20, 2022 •

edited

Loading

Yongxuanzhang commented Jul 20, 2022 •

edited

Loading

Yongxuanzhang Jul 20, 2022 •

edited

Loading

Yongxuanzhang commented Jul 21, 2022 •

edited

Loading