-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
WIP: chore(actions): support cron schedule task #22751
Conversation
I have an alternative solution for reference: Define tables like: type ActionSchedule struct {
ID int64
Title string
Specs []string
// and other necessary fields, without EntryIDs
}
type ActionScheduleSpec struct {
ID int64
ScheduleID int64
Spec string
} And the data looks like: ActionSchedule:
ActionScheduleSpec:
Then get rid of
So we don't have to do something like |
I don't understand why you're using robfig's cron instead of using the inbuilt cron within gitea itself? |
@zeripath I will remove the robfig's cron package and refactor the PR, get back to you asap. |
Codecov Report
@@ Coverage Diff @@
## main #22751 +/- ##
==========================================
- Coverage 47.14% 47.04% -0.10%
==========================================
Files 1149 1159 +10
Lines 151446 152589 +1143
==========================================
+ Hits 71397 71789 +392
- Misses 71611 72325 +714
- Partials 8438 8475 +37
... and 12 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
services/actions/schedule_tasks.go
Outdated
} | ||
|
||
func startTasks(ctx context.Context, opts actions_model.FindSpecOptions) error { | ||
specs, count, err := actions_model.FindSpecs(ctx, opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a big instance, how many the records do you think it will contain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any better ideas? I think we have to fetch all specs and check them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we fetch all before checking them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we could get all ids into memory and then fetch by pagination.
services/actions/schedule_tasks.go
Outdated
} | ||
|
||
func startTasks(ctx context.Context, opts actions_model.FindSpecOptions) error { | ||
specs, count, err := actions_model.FindSpecs(ctx, opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any better ideas? I think we have to fetch all specs and check them.
I think this could be refactored after https://gitea.com/gitea/act/pulls/29 merged. |
I've just resolved the conflicts |
@nephatrine While I feel absolutely the same way about it, 1.20 is out of the picture. We are already in feature freeze, meaning no new features will be accepted for it anymore. |
…25716) - cancel running jobs if the event is push - Add a new function `CancelRunningJobs` to cancel all running jobs of a run - Update `FindRunOptions` struct to include `Ref` field and update its condition in `toConds` function - Implement auto cancellation of running jobs in the same workflow in `notify` function related task: #22751 --------- Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com> Signed-off-by: appleboy <appleboy.tw@gmail.com> Co-authored-by: Jason Song <i@wolfogre.com> Co-authored-by: delvh <dev.lh@web.de>
Sorry for the noise, but I'm so ready for this to be merged, in order to move in a couple of final repos from GitHub into Gitea (I've been following the PR for a few months now)! 😅 Thank you so much for the hard work here @appleboy ! |
Could you add some unit test cases. |
@appleboy The merged auto-cancellation is only for push events, not for cron, so it won't help us here - or am I wrong? |
Will this make it into gitea soon, or should I look into woodpecker ci or similar? |
Happy to rubber-stamp this once the merge conflicts and CI failures are fixed. |
Could somebody clarify the state of this PR? IIRC the merged auto-cancellation of jobs only applies to push events. The blocker for this PR was that a wrongly configured cron task could launch many concurrent jobs. So is this PR still blocked, do we not consider this an issue anymore or is there another solution? |
models/actions/schedule_spec_list.go
Outdated
e.Limit(opts.PageSize, (opts.Page-1)*opts.PageSize) | ||
} | ||
var specs SpecList | ||
total, err := e.Desc("id").FindAndCount(&specs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there is only on call to FindSpecs
and it doesn't need "count"
go.mod
Outdated
@@ -53,6 +53,7 @@ require ( | |||
github.com/go-webauthn/webauthn v0.8.6 | |||
github.com/gobwas/glob v0.2.3 | |||
github.com/gogs/chardet v0.0.0-20211120154057-b7413eaefb8f | |||
github.com/gogs/cron v0.0.0-20171120032916-9f6c956d3e14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC gogs/cron
had been removed, now it is back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is a block change. It needs to be changed to the new cron package or you could write the parser yourself. @appleboy
Sorry, I made a mistake. Please follow #26655 |
Replace #22751 1. only support the default branch in the repository setting. 2. autoload schedule data from the schedule table after starting the service. 3. support specific syntax like `@yearly`, `@monthly`, `@weekly`, `@daily`, `@hourly` ## How to use See the [GitHub Actions document](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule) for getting more detailed information. ```yaml on: schedule: - cron: '30 5 * * 1,3' - cron: '30 5 * * 2,4' jobs: test_schedule: runs-on: ubuntu-latest steps: - name: Not on Monday or Wednesday if: github.event.schedule != '30 5 * * 1,3' run: echo "This step will be skipped on Monday and Wednesday" - name: Every time run: echo "This step will always run" ``` Signed-off-by: Bo-Yi.Wu <appleboy.tw@gmail.com> --------- Co-authored-by: Jason Song <i@wolfogre.com> Co-authored-by: techknowlogick <techknowlogick@gitea.io> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@yearly
,@monthly
,@weekly
,@daily
,@hourly
How to use
See the GitHub Actions document for getting more detailed information.
Signed-off-by: Bo-Yi.Wu appleboy.tw@gmail.com