Parameters should default to type string

[jerop]

Expected Behavior

Pipelines with Parameters that have no "type" specified work with the default value of "string".

Actual Behavior

Pipelines with Parameters that have no "type" fail.

Example failures in dogfooding are here: https://dashboard.dogfooding.tekton.dev/#/namespaces/default/pipelineruns

Steps to Reproduce the Problem

1. Create a Pipeline with a Parameter without a default value specified.
2. Run the Pipeline
3. Observe the PipelineRun logs.

Additional Info

Ran into this error when running the release Pipeline which uses the default types for Parameters - https://github.com/tektoncd/pipeline/blob/94055d92c120a6010f3d61a821e45dea4f893a74/tekton/release-pipeline.yaml

This was also observed in other Pipelines in dogfooding - tektoncd/plumbing#1148

A related issue with types in Results was fixed in #5043 and patched in release v0.37.1

• Kubernetes version:

  Output of kubectl version:

  Client Version: v1.24.1
  Kustomize Version: v4.5.4
  Server Version: v1.21.12-gke.1500
  

• Tekton Pipeline version:

  Output of tkn version or kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'

  Client version: 0.24.0
  Chains version: v0.8.0
  Pipeline version: v0.37.1
  Triggers version: v0.20.1
  Dashboard version: v0.27.0
  

cc @Yongxuanzhang @chuangw6 @lbernick @dibyom @afrittoli @abayer

[jerop]

/assign @Yongxuanzhang

[tekton-robot]

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

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 @Yongxuanzhang

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.

[Yongxuanzhang]

/assign

[abayer]

For what it's worth, I would suggest we add a commit on the release-v0.37.x branch adding types to tekton/publish.yaml and tekton/release-pipeline.yaml so that we can actually release the branch with the fix for this cherry picked.

[abayer]

@jerop Ok, now you shouldn't actually need to worry about changing release-pipeline.yaml etc - it turns out that they were getting updated properly during the release process, but defaulting wasn't happening because the webhooks.knative.dev/exclude: "true" label was on the default namespace, skipping the webhooks entirely. 🤦 I just fixed that and existing Pipelines without param types properly got types added.

[Yongxuanzhang]

So can we close this issue? Very glad that we can find out the reason eventually. 😄

[abayer]

We still need the validation/defaulting/etc to kick in for unchanged Pipelines and Tasks, so no, the PR still needs to be done. =)

[Yongxuanzhang]

We still need the validation/defaulting/etc to kick in for unchanged Pipelines and Tasks, so no, the PR still needs to be done. =)

Could you elaborate more on unchanged Pipelines and Tasks? We're planing to add SetDefaults to reconcile so even without mutating webhook, we can also add missing values. Does this solution sounds good to you?

[abayer]

Right, that's exactly what I mean. I'm just saying this issue shouldn't be closed until #5176 merges.

[Yongxuanzhang]

Right, that's exactly what I mean. I'm just saying this issue shouldn't be closed until #5176 merges.

Yeah sure! Np!

[dibyom]

We still need the validation/defaulting/etc to kick in for unchanged Pipelines and Tasks, so no, the PR still needs to be done. =)

Yes, we do. But the type defaulting behavior is not new and has been around for more than a release. So I don't think this issue needs to block the release

[dibyom]

I think this can be closed now right @Yongxuanzhang @abayer ?

[Yongxuanzhang]

I think this can be closed now right @Yongxuanzhang @abayer ?

Yeah I think so.

[abayer]

Agreed