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-0128]: Scheduled Runs #904

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Conversation

lbernick
Copy link
Member

This commit splits TEP-0083 (scheduled and polling runs) into two TEPs, one for scheduled runs and one for polling runs, and adds an implementation plan for scheduled runs.

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Dec 20, 2022
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 20, 2022
@pritidesai
Copy link
Member

API WG - this is mainly triggers project related changes. Please bring this to triggers WG or Slack channel.
/assign @dibyom

teps/0128-scheduled-runs.md Show resolved Hide resolved

### Bindings

As part of future work for this proposal, we can add two new variable replacements in bindings:
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like this idea!

similar to the kubectl functionality `kubectl create job --from=cronjob/my-cronjob`, for example:

```sh
tkn eventlistener start <el_name>
Copy link
Member

Choose a reason for hiding this comment

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

+1 A more general purpose could be a nice addition too tkn eventlistener start -t trigger-name --body ${} etc

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea, added this example syntax

teps/0128-scheduled-runs.md Outdated Show resolved Hide resolved
@lbernick lbernick force-pushed the crontrigger branch 5 times, most recently from f6217ab to e6e5f83 Compare January 11, 2023 23:01
Comment on lines +77 to +78
triggers:
- triggerRef: experimental-release-pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have plan to support reference for Eventlistener as well ?

Thinking like CronTrigger will trigger based on given schedule so if ref is for Eventlistener then it will create Eventlistener object

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 check my understanding, you're wondering if we'd plan to support a reference to an EventListener within a CronTrigger, and the CronTrigger would send a fixed event to that EventListener on a schedule? I don't have plans to include that in a proposal but I'm curious if there's a use case you have in mind for that feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

No usecase as of now but just thought like if someone need to execute EL based on cronTrigger

But ya proposal can be enhanced in future if we get such use case scenario

teps/0128-scheduled-runs.md Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2023
@afrittoli
Copy link
Member

/assign @afrittoli

@lbernick
Copy link
Member Author

@afrittoli @savitaashture if you have time to take a look at this proposal I'd love to merge it during monday's WG!

This commit splits TEP-0083 (scheduled and polling runs) into two TEPs,
one for scheduled runs and one for polling runs, and adds an implementation plan
for scheduled runs.
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thanks @lbernick, this looks great to me.
I will leave it to the triggers team to decide whether this has enough details for the TEP to me marked as implementable. Perhaps a diagram of the CRDs involved and their relationship could be helpful, but maybe it's just me :)
/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, dibyom

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:

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

@lbernick
Copy link
Member Author

I will leave it to the triggers team to decide whether this has enough details for the TEP to me marked as implementable.

@savitaashture are you able to weigh in here?

@savitaashture
Copy link
Contributor

I will leave it to the triggers team to decide whether this has enough details for the TEP to me marked as implementable.

@savitaashture are you able to weigh in here?

Thanks @lbernick for putting the details thought 👍

@savitaashture
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2023
@tekton-robot tekton-robot merged commit 18db04c into tektoncd:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. 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.

6 participants