Proposal: Support . in param / result names

[mattmoor]

No description provided.

[bobcatfish]

is it possible to go into more detail around these? i dont totally understand the use cases myself - and I'd like to understand the use cases we have for this that wouldn't be satisfied by having dictionary/object support (TEP-0075) ?

[mattmoor]

They are completely unrelated. The motivation is to be able to define parameters to Tasks and Pipelines that enable a level of unambiguous matching of certain semantic characteristics. For instance, if a Task or Pipeline were parameterized by some workspace setup logic, being able to unambiguously identify that semantic through a highly scoped parameter name would be extremely useful. Ultimately this isn't about teaching tektoncd/pipelines any new tricks, but providing enough leeway that higher-level tooling (incl. potentially tkn) can create higher-level experiences.

I'm currently doing this extensively in mink, but with -, which leaves a lot to be desired. I enumerated a large number of potential use cases here: #504

[mattmoor]

I'm also happy to jump on a quick call if it'd be easier to talk through synchronously.

[bobcatfish]

For instance, if a Task or Pipeline were parameterized by some workspace setup logic, being able to unambiguously identify that semantic through a highly scoped parameter name would be extremely useful.

I'm still not understanding why having dictionary parameter support wouldn't give you that.

In #504 you show this example:

      params:
        - name: dev.mink.kontext
          description: A self-extracting container image of source

If we had dictionary support (eventually nested dictionary support) that could be expressed as:

      params:
       - name: dev
         description: A self-extracting container image of source
         type: object
         properties:
         mink: {
              type: object
              properties: {
                 kontext: {
                     type:string
                }
              } 
          }

It's certainly a lot longer, so maybe it's the verbosity that makes you want to prefer encoding the namespacing into the string? The above structure would let you define other properties as part of your mink object as well - and as @wlynch had mentioned we could have predefined schemas for known types (like "mink params").

[mattmoor]

I think the confusion is somewhat like conflating hierarchical package namespaces and nested object fields.

Yes, you could probably implement namespaces as very deeply nested objects, but it is both extremely verbose and obscures the fundamental motivation: unique naming.

Part of the point of this is to be an enabler for higher-level "ownership" conventions through things like domain-scoping, and in my example the presence of a domain is very clear, but in yours it is completely obfuscated. Similarly if Java had folks define:

class com {
   class google {
       class foo {
           class Bar {
           }
       }
   }
}

It would serve the purpose of enabling com.google.foo.Bar, but it gets especially rough when you consider that Java doesn't have open classes, so everything in com would have to go in that file! Similarly, everything in dev (and dev.mink) would have to be blended into a single shared schema, which also means these can't take advantage of things like SchemaRefs (assuming those eventually land) without a blending syntax.

[vdemeester]

I tend to side with @mattmoor there, it's just about the name, not nested object fields, …
I see value with this to "namespace" some parameters ; that could, indeed, be used by higher-level tooling. I see value for openshift (and openshift tooling / consule), to be able to have io.openshift.* set of params, etc..

[bobcatfish]

I guess I'm still confused about the use case where a Task is being written such there are so many params that they don't need to be grouped but ownership needs to be defined.

In the openshift example, I'd think we have one of two things happening:

1. A super generic task (e.g. git-clone or something) is being passed values that originated in some specific system (e.g. openshift) - in this case the task is still generic so I dont think it would have open shift specific params
2. A task specific to openshift - i this case, i dont really understand why you'd need to namespace the params at all if the entire Task was specific to openshift (but being able to group them seems useful which is where i keep coming back to dictionaries)

Maybe in the context of a pipeline it would make more sense - you'd be receiving params from a variety of sources, maybe identifying the ones generated by openshift would make sense - but i still dont understant why you woudnt want to group them, i.e. the difference between something like:

      params:
        - name: openshift.foo
        - name: openshift.bar
        - name: openshift.baz

and

      params:
       - name: openshift
         type: object
         properties:
           foo : { type: string}
           bar : { type: string}
           baz : { type: string}

But anyway even though I'm not convinced about the specific use cases we probably don't need to set the bar as high as I'm trying to just to allow some specific character in a param name 😆 , as long as we have an unambiguous way of referring to it in our syntax.

but in yours it is completely obfuscated.

I don't totally understand what you mean @mattmoor - the structure of the dictionary does express the same information?

Similarly, everything in dev (and dev.mink) would have to be blended into a single shared schema, which also means these can't take advantage of things like SchemaRefs (assuming those eventually land) without a blending syntax.

maybe you could explain a bit more about the single shared schema and the blending you're mentioning?

(it sounds like you're saying if one Task had a param with the top level key "mink" that would prevent other Tasks from having the top level key "mink" with a different structure? if so i dont follow why that's the case)

[mattmoor]

the structure of the dictionary does express the same information

To a machine capable of deep structural analysis of the type? yes for this example. At a glance, absolutely not (thus the term "obfuscate").

I also don't think keys like openshift and mink are unambiguous enough that we should expect common users to avoid defining them, where it's a pretty well-worn precedent that domains are the purview of the domain owner.

I also had a thought this morning, which I think will help shine a light on the confusion and distinction I am trying to draw between namespaced field names and decomposing those same fields into objects themselves (see). 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.

[bobcatfish]

hmm so what I'm taking away from this is that a different way of looking at this proposal might be something like: as we expand support for json based params, we should look at supporting more of the characters that json considers valid which we currently don't, including . (and / in the examples you added)

seems reasonable to me!

i tried to see if there was any reason why we are limiting the characters the way we are and it seems to come down to tektoncd/pipeline#472 where @vdemeester says:

I think this is a good "base" for variable name but might be too restrictive though

so it seems like it was arbitrary and expanding to include . makes sense (based on your example above would you want to include / as well? "knative.dev/controller" is a bit more readable for me personally than "dev.knative.controller" but that's just me)

[jerop]

/assign @jerop @bobcatfish @vdemeester

[vdemeester]

I tend to side with @mattmoor there, it's just about the name, not nested object fields, …
I see value with this to "namespace" some parameters ; that could, indeed, be used by higher-level tooling. I see value for openshift (and openshift tooling / consule), to be able to have io.openshift.* set of params, etc..

[bobcatfish]

Thanks for all the discussion @mattmoor - im still not 100% sure what Tasks will look that use these domain scoped params but overall im in favor of the proposal (esp from the perspective of expanding our json based param support) and maybe when I see it in action it'll make sense to me

/approve

[bobcatfish]

thanks for the additional info!

[bobcatfish]

👍

[bobcatfish]

hmm so what I'm taking away from this is that a different way of looking at this proposal might be something like: as we expand support for json based params, we should look at supporting more of the characters that json considers valid which we currently don't, including . (and / in the examples you added)

seems reasonable to me!

i tried to see if there was any reason why we are limiting the characters the way we are and it seems to come down to tektoncd/pipeline#472 where @vdemeester says:

I think this is a good "base" for variable name but might be too restrictive though

so it seems like it was arbitrary and expanding to include . makes sense (based on your example above would you want to include / as well? "knative.dev/controller" is a bit more readable for me personally than "dev.knative.controller" but that's just me)

[mattmoor]

👍 LMK if I can answer any more questions, or adjust any other aspects of this.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, vdemeester

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:

• teps/OWNERS [bobcatfish,vdemeester]

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

[bobcatfish]

@mattmoor it totally slipped my mind that I've actually got another use case for this feature - in the pipeline to task run experimental control i'm taking the params from a bunch of tasks and squishing them down into one task - so I try to namespace them by the name of the original pipeline task! I wanted to use . for the namespacing but went with - because . wasn't supported XD (it ends up looking like this: https://github.com/tektoncd/experimental/blob/main/pipeline-to-taskrun/pkg/reconciler/pipelinetotaskrun/testdata/expected-taskrun.yaml ). and thinking about it, i dont think it would be reasonable (at least initially) to put these into dictionaries instead b/c it would mean having to change the way the values are written within the steps

so anyway TL;DR if you're looking for another use case for the proposal I've got one and I'll totally use this feature once it's available

[dlorenc]

This just reminded me of the funny -dot- escaping App Engine used to do to avoid nested subdomains and double wildcard certs :)

[mattmoor]

Mostly a TEP-noobie process question: What's left to merge this?

I'd be happy to try and implement this next week, but I think the API meeting Monday is cancelled due to Labor day.

I don't want to put the cart before the horse, so figured I'd ask here. thanks, and enjoy the long weekend (if you have one)

[vdemeester]

@mattmoor 2 approve from 2 different company, we are just waiting for a lgtm from.. another owner and we are good to go 👼🏼

[dlorenc]

/lgtm

[bobcatfish]

btw link to the process @vdemeester and @dlorenc - we've been using an approach where approvers /approve and we do the final /lgtm in the API working group: https://github.com/tektoncd/community/tree/main/teps#tep-review-process-and-slos

[dlorenc]

It says here that anyone can add the final approval, given there was no API meeting this week for the holiday I felt it was fine:

Once all assigned reviewers have approved the PR, the final /lgtm can be added in the next API working group meeting or sooner by anyone with permission to if needed.

[bobcatfish]

ah good point @dlorenc 👍 thanks for looking and pointing that out

[bobcatfish]

oh you know what @mattmoor - technically we need to update this to "implementable" - technically this should have happened before tektoncd/pipeline#4215 went in

it's a weird part of our process - often after a lot of back and forth on the initial PR we just update it to "implementable" right from the start, which we should have probably remembered to suggest here 🤦‍♀️

at this point maybe you could just update the proposal with "implemented"?

[mattmoor]

Sure, TIL. Will send a PR shortly.

[mattmoor]

#518