TEP-0047: Update and move to implementable

[abayer]

This is rather straightforward as written - we just need to add DisplayName fields to PipelineSpec, TaskSpec, and PipelineTask, and add a Description field to PipelineTask.

Signed-off-by: Andrew Bayer [email protected]

[jerop]

/assign

[jerop]

Thank you for picking this up @abayer!

I'm hesitant about marking this TEP implementable when there are details that have not been fleshed out yet. At minimum, I suggest that we include some alternatives to the proposed solution and why we won't implement those alternatives. One alternative is using annotations as proposed by @sbwsg in #319 (comment).

[jerop]

There were some alternatives discussed in the original proposal, such as using annotations, could they please be added here?

[vdemeester]

For Pipeline and Task, I think the only value I see in those field is to be able to use them in variable interpolation — as today, we can't use annotations — but I am not sure what the use case would be here. If we don't have this need, an annotation just does the job, doesn't it ?

For PipelineTask (aka Task referenced in a Pipeline), the question is different 👼🏼

[abayer]

Ok, I need to think on this.

There's already a Description field on both Pipeline and Task, so unless we want to remove that and cause compatibility issues (which, frankly, I think would be more pain for users than any value that would come from moving to a standard tekton.dev/description annotation), those aren't going to change. So for Pipeline and Task, the question is whether to have both the Description field and a DisplayName field, or the Description field and a standardized tekton.dev/displayName annotation.

Then for PipelineTask, we either would add the Description and DisplayName fields, as proposed here, or add the existing PipelineTaskMetadata struct (used as a field in EmbeddedTask and EmbeddedRunSpec currently) as an optional Metadata field in PipelineTask.

I honestly don't know what's best here. I don't see any solution that isn't at least a bit confusing or problematic (i.e., removing Description from Pipeline and Task feels like a bad idea to me), and adding (pipeline task).metadata.annotations.etc... feels overly verbose to me. I'd lean towards just adding the fields as proposed here, so that there's a consistent way to specify DisplayName and Description for all of Pipeline, Task, and PipelineTask, but will wait to update the alternatives in the TEP until there seems to be a consensus here.

[jerop]

/kind tep

[jerop]

/assign @vdemeester

[vdemeester]

Ok, I need to think on this.

There's already a Description field on both Pipeline and Task, so unless we want to remove that and cause compatibility issues (which, frankly, I think would be more pain for users than any value that would come from moving to a standard tekton.dev/description annotation), those aren't going to change. So for Pipeline and Task, the question is whether to have both the Description field and a DisplayName field, or the Description field and a standardized tekton.dev/displayName annotation.

Then for PipelineTask, we either would add the Description and DisplayName fields, as proposed here, or add the existing PipelineTaskMetadata struct (used as a field in EmbeddedTask and EmbeddedRunSpec currently) as an optional Metadata field in PipelineTask.

I honestly don't know what's best here. I don't see any solution that isn't at least a bit confusing or problematic (i.e., removing Description from Pipeline and Task feels like a bad idea to me), and adding (pipeline task).metadata.annotations.etc... feels overly verbose to me. I'd lean towards just adding the fields as proposed here, so that there's a consistent way to specify DisplayName and Description for all of Pipeline, Task, and PipelineTask, but will wait to update the alternatives in the TEP until there seems to be a consensus here.

My initial thought on this is, "why do we need DisplayName ?" and "why are we the only one in need of this ?", aka why the Service type doesn't have that need for example ?
What DisplayName adds that cannot be "guessed" by an UI (replace - by a space, …) ?
On PipelineTask, it's a bit different, depending on if we use embedded Task or using a very generic Task.

All in all, I don't think having those in would be such a big deal either, it could be interesting if they could be used in variable interpolation ("be aware of injection", cc #596) but otherwise they seem harmless so… I can live with having those fields in 👼🏼

/approve
/cc @bobcatfish @sbwsg @pritidesai @abayer @jerop

[tekton-robot]

@vdemeester: GitHub didn't allow me to request PR reviews from the following users: abayer.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Ok, I need to think on this.

There's already a Description field on both Pipeline and Task, so unless we want to remove that and cause compatibility issues (which, frankly, I think would be more pain for users than any value that would come from moving to a standard tekton.dev/description annotation), those aren't going to change. So for Pipeline and Task, the question is whether to have both the Description field and a DisplayName field, or the Description field and a standardized tekton.dev/displayName annotation.

Then for PipelineTask, we either would add the Description and DisplayName fields, as proposed here, or add the existing PipelineTaskMetadata struct (used as a field in EmbeddedTask and EmbeddedRunSpec currently) as an optional Metadata field in PipelineTask.

I honestly don't know what's best here. I don't see any solution that isn't at least a bit confusing or problematic (i.e., removing Description from Pipeline and Task feels like a bad idea to me), and adding (pipeline task).metadata.annotations.etc... feels overly verbose to me. I'd lean towards just adding the fields as proposed here, so that there's a consistent way to specify DisplayName and Description for all of Pipeline, Task, and PipelineTask, but will wait to update the alternatives in the TEP until there seems to be a consensus here.

My initial thought on this is, "why do we need DisplayName ?" and "why are we the only one in need of this ?", aka why the Service type doesn't have that need for example ?
What DisplayName adds that cannot be "guessed" by an UI (replace - by a space, …) ?
On PipelineTask, it's a bit different, depending on if we use embedded Task or using a very generic Task.

All in all, I don't think having those in would be such a big deal either, it could be interesting if they could be used in variable interpolation ("be aware of injection", cc #596) but otherwise they seem harmless so… I can live with having those fields in 👼🏼

/approve
/cc @bobcatfish @sbwsg @pritidesai @abayer @jerop

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.

[jerop]

@abayer we discussed this at the API working group on 03/14 and agreed to support the display name in the spec, alongside the descriptions.

Please add these alternatives in the TEP:

• Using annotations for display names and description: we don't want to do this because as described in the Kubernetes API conventions, "Annotations are primarily generated and consumed by tooling and system extensions, or are used by end-users to engage non-standard behavior of components".
• Using labels for display names and description: this is a more viable option because as described in the Kubernetes API conventions, "Labels are the domain of users. They are intended to facilitate organization and management of API resources using attributes that are meaningful to users, as opposed to meaningful to the system. Users will use labels to select resources to operate on, display label values in CLI etc.". However, we'd have to deprecate the existing description fields in the spec.

With the above updates, the TEP should be good to go 🙏🏾

cc @itewk @AlanGreene @sbwsg @pritidesai @dibyom

[itewk]

woot woot. let me know if you need me to do anything.

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

[itewk]

this still alive?

[lbernick]

@abayer please rebase

API WG 6/13: good to go (anyone can add lgtm after PR is rebased)

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

[itewk]

ping

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop, 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 [jerop,vdemeester]

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

[vdemeester]

/remove-lifecycle rotten

[afrittoli]

Agreed during WG
/lgtm