Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Proposal: Support . in param / result names #503

Merged
merged 3 commits into from
Sep 6, 2021
Merged

Proposal: Support . in param / result names #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4993839
Proposal: Support `.` in param / result names
mattmoor Aug 19, 2021
818adde
Fix typo
mattmoor Aug 30, 2021
eb06efe
Replace quoting with subscript
mattmoor Aug 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 165 additions & 0 deletions teps/0080-support-domainscoped-parameterresult-names.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
---
status: proposed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, TIL. Will send a PR shortly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#518

title: Support domain-scoped parameter/result names
creation-date: '2021-08-19'
last-updated: '2021-08-19'
authors:
- '@mattmoor'
---

# TEP-0080: Support domain-scoped parameter/result names

<!-- toc -->
- [Summary](#summary)
- [Motivation](#motivation)
- [Goals](#goals)
- [Non-Goals](#non-goals)
- [Use Cases (optional)](#use-cases-optional)
- [Requirements](#requirements)
- [Proposal](#proposal)
- [Notes/Caveats (optional)](#notescaveats-optional)
- [Risks and Mitigations](#risks-and-mitigations)
- [User Experience (optional)](#user-experience-optional)
- [Performance (optional)](#performance-optional)
- [Design Details](#design-details)
- [Test Plan](#test-plan)
- [Design Evaluation](#design-evaluation)
- [Drawbacks](#drawbacks)
- [Alternatives](#alternatives)
- [Infrastructure Needed (optional)](#infrastructure-needed-optional)
- [Upgrade &amp; Migration Strategy (optional)](#upgrade--migration-strategy-optional)
- [Implementation Pull request(s)](#implementation-pull-request-s)
- [References (optional)](#references-optional)
<!-- /toc -->

## Summary

Allow Task and Pipeline authors to expose parameters and results that allow
`.` characters.

> _This TEP was originally raised [here](https://github.com/tektoncd/pipeline/issues/3590)
as an issue for discussion._

## Motivation

The motivation of this work is to *enable* us to establish conventions around the
definition of parameters and results that may have a deeper meaning for
higher-level systems without a high-degree of accidental naming collisions.

The pursuit of `.` is specifically to follow the precedents set by many other systems
(including Kubernetes labels, CloudEvent types, Java import paths, ContainerD plugins,
and many more) of using a Domain-scoped naming convention to "claim" a segment of
names for the owner's exclusive use. This is of course conventional, but in practice
is generally good enough.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the additional info!


With this work, higher-level systems could start to define Task/Pipeline "interfaces"
or leverage partial signature matching to enable special semantics around certain
signatures, without (significant) concerns around matching something accidentally.

> _It is notable that with @bobcatfish [TEP for stronger
typing](https://github.com/tektoncd/community/pull/479) that these conventions should
also establish the types associated with those parameter names, and might form a good
case for SchemaRefs._
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


### Goals

Enable conventions to be established around the use of domain-scoped names as a way
for the domain owner to "own" the definition of what those parameters and results are
for and how they will be used.

### Non-Goals

It is not a goal of this TEP to establish these conventions, or begin to define any
"well known" parameters or results owned by Tekton (e.g. `dev.tekton.foo.bar`).


### Use Cases (optional)

See the [original issue](https://github.com/tektoncd/pipeline/issues/3590) for some
of these.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


There are potentially a lot more things like this, but I'd rather leaves those for
a conversation around the types of conventions we should standardize, and not rat hole
here (just about enabling that next convo).

## Requirements

Parameters and Result name resolution must be unamiguous, especially in the presence
of proposals like [TEP for stronger typing](https://github.com/tektoncd/community/pull/479),
which allows folks to access fields of complex parameters.

## Proposal

Two parts to the proposal:

1. Allow folks to define parameters and results with `.` in the name:
```yaml
params:
- name: dev.mattmoor.foo
bobcatfish marked this conversation as resolved.
Show resolved Hide resolved
```

2. Allow folks to reference parameters and results with subscript around the name
(required if it contains a `.`):
```yaml
steps:
- image: $(params[dev.mattmoor.foo])
```

### Notes/Caveats (optional)

### Risks and Mitigations

The main risk is likely the ambiguity of references without the quoting requirement,
especially in a world where parameter and results can themselves be object and parts
of those are accessed via `.`

### User Experience (optional)

This likely doesn't affect UX much beyond it needing to support the expanded set
of names. Ultimately, this could enable higher-level UX's that are (IMO) much
better than the status quo.

### Performance (optional)

N/A

## Design Details

This mostly feels like a plumbing exercise, but I'd be happy to expand if there are
specifics worth fleshing out in advance of PRs.

## Test Plan

Testing should be added of each of the pieces above: quoting (alone), and quoting
of parameters and results with `.` in both Task and Pipeline contexts.

## Design Evaluation

Conformant Tekton implementations should support this.

## Drawbacks

The need to quote parameters names may not come intuitively to Task authors, but
if this becomes a well established precedent that's adopted in places like the
catalog there will be ample examples demonstrating how to use this properly.

## Alternatives

We could establish conventions around non-domain names (such as `s/[.]/-/`), but this
feels like a less natural convention given the strong precedent for domains.

## Infrastructure Needed (optional)

N/A

## Upgrade & Migration Strategy (optional)

I can't think of any problems, since this isn't supported today.

## Implementation Pull request(s)

Not there yet!

## References (optional)

[Original issue](https://github.com/tektoncd/pipeline/issues/3590)
1 change: 1 addition & 0 deletions teps/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,3 +221,4 @@ This is the complete list of Tekton teps:
|[TEP-0071](0071-custom-task-sdk.md) | Custom Task SDK | proposed | 2021-06-15 |
|[TEP-0072](0072-results-json-serialized-records.md) | Results: JSON Serialized Records | implementable | 2021-07-26 |
|[TEP-0073](0073-simplify-metrics.md) | Simplify metrics | proposed | 2021-06-23 |
|[TEP-0080](0080-support-domainscoped-parameterresult-names.md) | Support domain-scoped parameter/result names | proposed | 2021-08-19 |