Support for params with .s

[mattmoor]

Expected Behavior

A way to use params with .s

Actual Behavior

No way to reference params with .s

Steps to Reproduce the Problem

apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
  name: hello
spec:
  params:
    - name: dev.mattmoor.foo
  steps:
    - name: echo
      image: ubuntu
      command: ["/bin/bash", "-c"]
      args:
        - |
          echo $(params."dev.mattmoor.foo")

Additional Info

I tested this against github.com/mattmoor/mink, so it may be a latent downstream bug, but I've got the validation wired up, so I was surprised this was accepted if it doesn't work.

[mattmoor]

Another take on this is "validation hole!", but let me explain why I want domain-style parameter names...

Tekton Tasks and Pipelines take inputs (params) and emit outputs (results), and so if you think about it a *Run is a long-running API call of sorts. Since I've never met an API I didn't quack at, my first though is ofc: duck types!

More seriously, as Task and Pipeline definitions start to sprawl, it becomes more interesting to establish categories and reason about them in terms of categories vs. case-by-case (this is why we started to pursue duck-typing as a way of reasoning about CRDs). So how do we establish the categories?

Taking a look at Task/Pipeline signatures, if we could establish an unambiguous way of naming key parameters, e.g.

  params:
    - name: dev.mink.bundle
      description: A self-extracting container image containing a source bundle
      value: tianon/true # default to the empty context, omit to require a source bundle.

... and results:

  results:
    - name: dev.tekton.image-digest
      description: The digest of the image produced by this resource.

Then we can start to activate and mix-in client capabilities with some rudimentary signature analysis.

---

I'm receptive to discussing other conventions as well, but DNS for disambiguation is fairly commonplace and well-understood by users.

[mattmoor]

Another examples of this, which seems especially relevant to syntax precedence, is containerd plugins:

  [plugins."io.containerd.grpc.v1.cri".containerd]
    disable_snapshot_annotations = true

If we were to follow this precedent, then it would suggest allowing: $(params."this.is.one.name") syntax.

However, this precedent does double-duty, since it uses reverse-DNS (like Java imports, and CloudEvent types).

I'll take a crack as allowing the param."foo" syntax to see how far I can get.

/assign

[mattmoor]

I noticed this in my triage of params.%s and don't see much context for why:

// convertParamTemplates replaces all instances of $(params.x) in the container to $(inputs.params.x) for each param name.
func convertParamTemplates(step *v1beta1.Step, params []v1beta1.ParamSpec) {
	replacements := make(map[string]string)
	for _, p := range params {
		replacements[fmt.Sprintf("params.%s", p.Name)] = fmt.Sprintf("$(inputs.params.%s)", p.Name)
		v1beta1.ApplyStepReplacements(step, replacements, map[string][]string{})
	}

	v1beta1.ApplyStepReplacements(step, replacements, map[string][]string{})
}

@vdemeester based on your FIXME comments, this seems like it's moving things the wrong direction. Is this still needed?

I'm going to ignore it for now.

[ghost]

Taking a look at Task/Pipeline signatures, if we could establish an unambiguous way of naming key parameters [...] and results [...] Then we can start to activate and mix-in client capabilities with some rudimentary signature analysis.

I broadly follow the idea I think but wonder why the categories would be tied to fixed names and not to some additional typing info that a Task author declares as part of a Result or Param. ex: "my task takes a git sha, an image url and a digest and then builds and pushes a new image, emitting a new url and digest". Each of those inputs and outputs could be individually validated by Tekton and clients could be responsible for recognizing the signature (gitsha, imageurl, digest) -> (imageurl', digest').

We've talked internally a tiny bit about annotating params and results with this kind of typing metadata. This was in the context of Tekton Chains (though Chains does lean on fixed names - e.g. IMAGE_URL, IMAGE_DIGEST - or use of PipelineResources which in turn emit those known names). Pinging @dlorenc here who will be better informed than I am about requirements and existing landscape.

To me, naming conventions feel like the domain of a higher-level tool than Pipelines since one client's desired space of names could differ from another's - and Pipelines wants to support everyone. Typing metadata feels like a general purpose tool that all clients could leverage in their own way but I'm possibly off-base in my understanding of what's actually desirable for a broad cross-section of tools building on top of Tekton?

[mattmoor]

To me, naming conventions feel like the domain of a higher-level tool than Pipelines

Right, the intent is not defining semantics at the level of Pipelines, but a level of support at the Pipelines level to enable higher-level conventions in a sensible way. One nice thing about the DNS form is that it naturally supports plurality when a thing might have higher arity (e.g. io.mattmoor.thing.{name}). It is also pretty widely accepted as a way of "claiming" a namespace.

That said, I could see Tekton defining certain upstream conventions that are used in tkn and to which the catalog tasks aim to adhere (e.g. dev.tekton.images.url, dev.tekton.images.digest), but again Pipelines should avoid sensitivity to the particulars as it is the lower-level abstraction.

I take advantage of this to a small degree in github.com/mattmoor/mink, and demoing for the tkn folks is on my short list for 2021 (the WG time is prohibitive for me in general).

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[vdemeester]

/lifecycle frozen
I think this needs to be taken into account or at least discussed 🙃

[bobcatfish]

Had a discussion about this in slack a bit, folks pointed out that the syntax for this might get a bit confusing, e.g. if we support results names with . we can end up with things like $(results.foo.bar.baz.path) (imagining a result named foo.bar.baz). I think the way our variable replacement is implemented this isnt technically difficult, but it does look strange.

I'm not sure that's enough of a reason not to do it - however at the same time it feels like maybe a better solution would be to support dictionary types? ala #1393

[mattmoor]

@bobcatfish Ack on the ambiguity. I think this could be addressed by something like this precedent in containerd: #3590 (comment)

I'm going to move this into a TEP, which seems like a better place to discuss the various aspects. Mea culpa for not doing that in the first place.

[mattmoor]

Closing this in favor of the linked TEP.