Skip to content

Conversation

@lbernick
Copy link
Member

CustomRun is different from other Tekton APIs because its API is implemented by custom run controllers, rather than in-tree controllers. Therefore, adding new fields to this API should be done with extra caution. This commit adds a design principle providing guidance on whether a new field should go in the CustomRun API or left to custom run controllers to support as part of the custom spec or custom status, based on whether a feature is applicable to all CustomRuns or just a feature of some of them.

CustomRun is different from other Tekton APIs because its API
is implemented by custom run controllers, rather than in-tree
controllers. Therefore, adding new fields to this API should
be done with extra caution. This commit adds a design principle
providing guidance on whether a new field should go in the CustomRun
API or left to custom run controllers to support as part of the
custom spec or custom status, based on whether a feature is
applicable to all CustomRuns or just a feature of some of them.
@tekton-robot tekton-robot requested review from dibyom and wlynch April 18, 2023 13:34
@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 dibyom after the PR has been reviewed.
You can assign the PR to them by writing /assign @dibyom 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 added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 18, 2023
@lbernick
Copy link
Member Author

lbernick commented Apr 18, 2023

/assign @jerop @pritidesai I'd love your feedback on this-- based on previous customrun discussions and jumping off discussion on tektoncd/pipeline#6532 (comment)

@tektoncd/core-maintainers FYI, would appreciate any feedback here

Motivation here was the fact that we decided not to include podtemplate in v1beta1.CustomRun (https://github.com/tektoncd/community/blob/main/teps/0114-custom-tasks-beta.md#remove-pod-template)

1. [Avoid implementing templating logic](https://docs.google.com/document/d/1h_3vSApIsuiwGkrqSiegi4NVaYG4oVzBquGAhIN6qGM/edit#heading=h.6kxvcvm7rs3r); prefer variable replacement.
1. Avoid implementing our own expression syntax; when required prefer existing languages which are widely used and include supporting development tools.
1. In TEPs, discuss how the proposal affects the flexibility of Tekton and demonstrate that any specific/opinionated choices are necessary but extensible.
1. Before adding an optional field to the spec or status of `CustomRun`, consider
Copy link
Member

Choose a reason for hiding this comment

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

This feels very specific to Pipelines. Perhaps somewhere in the pipelines documentation is a better place for this?

Maybe https://github.com/tektoncd/pipeline/tree/main/docs/developers?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, Triggers' ClusterInterceptor is also similar in that we except external users to implement instead of just in-tree implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I was trying to think of a more general way to express this since I know we have multiple api extension points in tekton projects but I think it'll be too confusing/generic-- I'll open a PR for this in pipelines instead.

@lbernick lbernick closed this Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants