Disallow using dots(.) in param names

[chuangw6]

Proposal: Disallow using dots(.) in param names

This doc mentioned that the parameter names should follow the following rules. (But we do not enforce it currently - reported here #4792 )

• Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.).
• Must begin with a letter or an underscore (_).

But we need to disallow the usage of dots(.) in param names.

Reason:

TEP-0075 proposes supporting object/dictionary type parameters that will have keys defined in properties section. See the parameter foo in the following example. And to reference a particular field in this object parameter we can use dot notation i.e. $(params.foo.key1), which is mentioned here.

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  generateName: object-param-test-
spec:
  taskSpec:
    params:
      - name: foo
        properties:
          key1: {}
          key2: {}
        default:
          key1: "val1"
          key2: "val2"
      - name: foo.key1
        default: "tricky"
    results:
      - name: echo-output
    steps:
      - name: echo-params
        image: bash
        script: |
          set -e
          echo $(params.foo.key1) | tee $(results.echo-output.path)

This reference approach for a key of the object parameter will have conflicts when it appears together with a parameter whose name contains dot(.) such as foo.key1. For example, when we use $(params.foo.key1), it's impossible to know whether it's referencing the particular key key1 of the object parameter named foo OR it's referencing a standalone parameter named foo.key1. This confusion is caused by the fact that containing a dot in parameter name is allowed currently.
Therefore, we should not allow to use dot(.) in parameter names.

[dibyom]

Hmmm...given that the dot notation this is already part of the beta API, I'm not sure we can immediately remove it given our deprecation policy: https://github.com/tektoncd/pipeline/blob/main/api_compatibility_policy.md#backwards-incompatible-changes

But I do agree - without this object params is tricky

/cc @vdemeester @wlynch @skaegi @jerop

[vdemeester]

@dibyom so this is a weird situation. We do document params shouldn't include dots (.) but we do not enforce it. We could enforce it and say "it was documented forever" but I tend to think it might break users and thus we would need to follow the deprecation policy.

It also reminds me of #3590.

[ywluogg]

There are two checkings for $(params.foo.key1): one is a param called foo.key1 and another understanding is that we are referring to key1 in a param foo. I agree with the direction that this is confusing, and probably disallowing this in the names should be the way we go. Before that, it is possible to validate against both of them and the reference to key1 in a param foo should be first considered. If neither of them being found, we return error. I think this will bypass the deprecation rule. Then we can worry about deprecation of this naming in params. This also applies to results.

[imjasonh]

It also reminds me of #3590.

cc @mattmoor

I'd definitely prefer not to break users, even if they're technically doing something that was recommended against but actually supported.

Since object-type results/params are coming, I think we could just support .s (and document they're supported) until the better fully supported feature exists.

[mattmoor]

This was already discussed extensively in the TEP to support dots: tektoncd/community#503. This comment in particular is relevant: tektoncd/community#503 (comment), quoting the relevant excerpt:

If the plan with TEP 75 is to allow for structured params and results following JSON structure, then this TEP devolves to handling fields within those same objects with . in them (and JSON fields can have a lot more characters Tekton likely doesn't support today!). My point is that the same trick doesn't work here (from my linked comment):

        name: line
        type: object
        properties:
          "knative.dev/controller": { type: string}
          "knative.dev/key": { type: string}
          ...

... this TEP is really just a degenerate case of this in a world where only primitive types are supported.

[chuangw6]

@dibyom so this is a weird situation. We do document params shouldn't include dots (.) but we do not enforce it. We could enforce it and say "it was documented forever" but I tend to think it might break users and thus we would need to follow the deprecation policy.

It also reminds me of #3590.

Hi @vdemeester ,
can you please point to me the documentation saying that params shouldn't include dots (.)?
Thanks!

[chuangw6]

It turns out there is no conflicts for such case because param names that contain dots can only be referenced with bracket notation. Updated in the TEP-0075