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.

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

[TEP-0081] Proposal: Convention for param/result reserved names #504

Closed
wants to merge 4 commits into from

Conversation

mattmoor
Copy link
Member

This builds on #503 to actually define a convention for parameter and result names.

/hold

Putting a hold since this is dependent on the other PR (separate commits).

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2021
@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 19, 2021
@vdemeester vdemeester added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Aug 23, 2021
@jerop
Copy link
Member

jerop commented Aug 23, 2021

/assign @jerop @bobcatfish @vdemeester

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2021
such as Chains can use this duck type to determine the attestation payload, and
other duck types such as `2.` to determine the artifact to which that attestation
applies (we could also have `dev.tekton.chains.subject`, but that's beyond the
scope of this TEP!).
Copy link
Contributor

Choose a reason for hiding this comment

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

i think asking Tasks to produce chains specific outputs introduces some coupling between specific Tasks and Pipelines and chains that it would be nice to avoid

maybe im getting caught up on the name, maybe if it was tekton.attestion instead of tekton.chains.attestation it would resonate with me more

the point is that chains is just one possible tool that could be observing this data - i think we should avoid introducing coupling between chains and pipelines if we can

Copy link
Member Author

Choose a reason for hiding this comment

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

To be completely clear: there should be zero code in tektoncd/pipeline that is aware of these conventions, but it should support users expressing them (thus the . TEP, but that's where it should end).

The point is to enable Task/Pipeline authors to target certain conventions (think: implement an interface or duck type) in order to hook into a semantic of a higher level system.

Whether chains is in the string or not, is a bikeshed out of scope for this TEP, this TEP is strictly about using reverse-domain scoping. This example is purely illustrative.


volumes:
- name: empty-dir
emptyDir: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

side note about volumes and volumemounts, workspaces provide a way to make it easy to provide the actual volumes at runtime instead of requiring the volume be declared in the Task

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack. I don't want to paint the bikeshed on illustrative examples, but I'm sure I could come up with (perhaps contrived) examples where each is "better" than the other in some regard.

One alternative would be to claim a namespace for Tekton, and not expose a more
generalized way to reserve names. Given that the only instance I've seen of this
being proposed was `tekton.*` I don't see why generalizing to the reverse domain
`dev.tekton.*` is harmful at all.
Copy link
Contributor

Choose a reason for hiding this comment

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

im guessing that the reverse domain approach has a lot of precedent - maybe it would be helpful to point to some?

having less exposure to these, from my perspective dev.tekton confuses me when i read it b/c i start wondering "what is dev" and additionaly wondering if it is some kind of stability indication (e.g. is there some kind like v1.tekton)

one (maybe minor) downside i can think of is what if we want to establish some kind of namespace that doesnt map to a some specific domain name

(and would dev.tekton.chains imply that chains.tekton.dev should resolve to something?)

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 thought I did, but off the top of my head:

  • Java imports
  • CloudEvent types
  • containerd plugins

(I'll make sure these land in the TEP, but I thought they were there 🤔 )

and would dev.tekton.chains imply that chains.tekton.dev should resolve to something

As with K8s apiGroup this is a subjective thing, but a nice thing folks could elect to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are totally right. I think I spaced on including these as I was plowing through the sections of the template. I'll rebase and push a list of these now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I added it under "Notes / Caveats" for now, lmk if there's someplace better, but I figured with it being right under "Proposal" it fit.

## Design Evaluation

This doesn't really directly influence Task/Pipeline conformance because this is a
convention on top of them. However, this does create new microcosms of conformance
Copy link
Contributor

Choose a reason for hiding this comment

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

would this be enforced somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whether it is even possible to create a conformance test is impossible to say outside of the context of a specific semantic, and this TEP isn't about defining any of these semantic interfaces, simply around laying the foundation to enable them.

I suspect for certain semantics, it could be possible to test certain point aspects of conformance (like "this Task emits results that are compatible with what something like Chains would sign")

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign bobcatfish
You can assign the PR to them by writing /assign @bobcatfish in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2021
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Very naive question, what differs from #503 ? Is it just about "making" a convention out of what would results from #503 ?

teps/0080-support-domainscoped-parameterresult-names.md Outdated Show resolved Hide resolved
@mattmoor
Copy link
Member Author

Is it just about "making" a convention out of what would results from #503 ?

Yes, #503 is just about allowing ., this is about establishing a convention akin to the use of domains in K8s labels/annotations or reverse-domain for Java packages et al.

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 7, 2021
@mattmoor
Copy link
Member Author

mattmoor commented Sep 7, 2021

/hold cancel

I have rebased this now that TEP-0080 has landed, and added the precedent of OCI annotations. This should be RFAL.

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2021
@bobcatfish
Copy link
Contributor

@mattmoor I'm not totally understanding what it would mean to accept this TEP - i.e. what would we actually DO? (e.g. publish some docs, maintain a list of reserved scopes?) if I missed this info in the PR plz just point me in the right direction

@mattmoor
Copy link
Member Author

@bobcatfish Yeah, good question.

It could be docs, but in some sense I see this (and acknowledgement of this agreement by some of the core stewards) as the sort of core meta artifact.

Beyond the scope of this, I think the subsequent artifacts (owned by Tekton) will be the sorts of conventions Tekton adopts using this (under dev.tekton.*), and how those manifest in the various repos (for instance, I think Catalog will have a strong voice in shaping those!).

@tekton-robot
Copy link
Contributor

@mattmoor: PR needs rebase.

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.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2021
@bobcatfish
Copy link
Contributor

I think the subsequent artifacts (owned by Tekton) will be the sorts of conventions Tekton adopts using this (under dev.tekton.*)

Do you have any examples of what these artifacts might look like?

(for instance, I think Catalog will have a strong voice in shaping those!).

If you have any ideas of specifically how this would impact the catalog (maybe an example of an existing Task that would benefit) I think that would be great to include

I'm not sure if this is the kind of thing you have in mind, but a couple important "schemas" I think Tekton should define and care about are: details when an image has been built and params that are needed when you clone a git repo (and probably outputs as well to reflect what was actually cloned). Is that the kind of thing you image using something like dev.tekton for? e.g. if you're producing an image artifact, (borrowing the object syntax from TEP-0075) something like:

     results:
       - name: "dev.tekton.builtImage"
         type: object
         properties:
           url:
            type: string
           digest:
            type: string

(nit could we do "tekton.dev/buildImage" instead...)

@mattmoor
Copy link
Member Author

Do you have any examples of what these artifacts might look like?

I'd say a mix of TEPs, and (for those) docs so it is clear how folks integrate with conventions like these.

I'm not sure if this is the kind of thing you have in mind

Yeah, images that are produced are exactly one of the cases I had in mind, and probably one of the first cases we should tackle because the pattern matching Chains has to do today is extremely janky 😅

I totally agree that TEP-0075 would be very complementary here, and avoid an explosion of things that could be objects, but I suspect we will quickly yearn for SchemaRefs 😁

could we do "tekton.dev/buildImage" instead

It's certainly an option. K8s does this, but has limits like single-slash. One thing I think we should think about (it can be here or elsewhere) is how we deal with higher-arity things. To be fair, the answer here may just be TEP-0075, most of my thinking on this aspect predates that.

One of the things I had in mind was that folks could use dev.tekton.build-image if a single image was produced, but we could also use dev.tekton.build-image.<name> to support multiple named outputs (maybe both if one is the "main" output).

@bobcatfish
Copy link
Contributor

I'd say a mix of TEPs, and (for those) docs so it is clear how folks integrate with conventions like these.

I'd still like to see something more concrete as a deliverable but maybe agreeing that we think this direction makes sense is enough for now?

One of the things I had in mind was that folks could use dev.tekton.build-image if a single image was produced, but we could also use dev.tekton.build-image. to support multiple named outputs (maybe both if one is the "main" output).

hm it'd be nice if we could find a way to not start encoding even more structure into these reserved names - it feels like an array of dev.tekton.build-image objects is probably what we'd really want here?

Thinking about it more maybe dev.tekton.build-image isn't the way we'd want to go for chains - I think we'd want to have some kind of duck typing vs. requiring any specific name for the result, do you agree? e.g. if you produce an object with the properties image-urland image-digest we assume you've built an image 🤔 🤔 🤔

I think I'd have an easier time wrapping my head around this TEP if we were trying to agree on structure for some specific examples, e.g. what interface Chains will be using an expecting - that's probably a separate TEP but I'm just still not totally understanding what we'd be agreeing to here with this TEP approved - without the TEP there's nothing to stop anyone from using this kind of naming scheme.

@jerop jerop removed their assignment Oct 7, 2021
@mattmoor
Copy link
Member Author

Listening to Solarwinds talk, one things that came to mind might be to have a standard result name that can be consumed to get turned into a github check / commit status by a separate observer controller (like Chains is)

@bobcatfish
Copy link
Contributor

re the comments ive left above my biggest question is if that interface is defined by the name of the results or if it's by the shape of the results

e.g.

     results:
       - name: "dev.tekton.builtImage"
         type: object
         properties:
           url:
            type: string
           digest:
            type: string

vs

     results:
       - name: "lotsOfImageInfo"
         type: object
         properties:
           dev.tekton.url: #maybe the namespaced idea still makes sense for the fields that fulfill the "duck typing"?
            type: string
           dev.tekton.digest:
            type: string
           phaseOfTheMoon:
            type: string
           taskSpecificStuffImInterestedIn:
            type: string

In the first example chains/github check/whatever would be looking for a result with an explicit name, in the second example, chains would be looking for fields within a result

@tekton-robot
Copy link
Contributor

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 tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 10, 2022
@tekton-robot
Copy link
Contributor

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.

@tekton-robot tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 9, 2022
@pritidesai
Copy link
Member

@mattmoor any updates or comments?

/lifecycle frozen

@tekton-robot tekton-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Feb 14, 2022
@mattmoor mattmoor closed this Feb 15, 2022
@bobcatfish bobcatfish changed the title Proposal: Convention for param/result reserved names [TEP-0081] Proposal: Convention for param/result reserved names Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: UnAssigned
Development

Successfully merging this pull request may close these issues.

7 participants