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

WIP: chore(actions): support cron schedule task #22751

Closed
wants to merge 0 commits into from

Conversation

appleboy
Copy link
Member

@appleboy appleboy commented Feb 4, 2023

  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 for getting more detailed information.

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

models/actions/schedule.go Outdated Show resolved Hide resolved
models/actions/schedule.go Outdated Show resolved Hide resolved
models/actions/schedule.go Outdated Show resolved Hide resolved
models/actions/schedule.go Outdated Show resolved Hide resolved
services/actions/notifier_helper.go Outdated Show resolved Hide resolved
services/actions/schedule.go Outdated Show resolved Hide resolved
services/actions/schedule.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 16, 2023
@wolfogre
Copy link
Member

wolfogre commented Feb 16, 2023

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:

ID Title Specs other fields
1 A * 2 * * *, * 3 * * * ...
2 B * 4 * * * ...

ActionScheduleSpec:

ID ScheduleID Spec
1 1 * 2 * * *
2 1 * 3 * * *
3 2 * 4 * * *

Then get rid of robfig/cron, just use the current cron frame work of Gitea. We need only one cronjob:

  • It runs once per minute.
  • Load all rows of ActionScheduleSpec from DB, I think it's fine when there are only 3 fields. Or we can have a cache for them.
  • Check Spec of ActionScheduleSpec, record theScheduleID if the spec matches the present time.
  • Load ActionSchedule from DB by the IDs recorded.
  • Create tasks for them.

So we don't have to do something like resetSchedule or schedule.Remove(row.EntryIDs).

@zeripath
Copy link
Contributor

I don't understand why you're using robfig's cron instead of using the inbuilt cron within gitea itself?

@appleboy
Copy link
Member Author

@zeripath I will remove the robfig's cron package and refactor the PR, get back to you asap.

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Merging #22751 (a693fe6) into main (f521e88) will decrease coverage by 0.10%.
The diff coverage is 34.12%.

❗ Current head a693fe6 differs from pull request most recent head 95320e8. Consider uploading reports for the commit 95320e8 to get more accurate results

@@            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     
Impacted Files Coverage Δ
cmd/dump.go 0.67% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/actions/schedule_list.go 0.00% <0.00%> (ø)
models/actions/schedule_spec_list.go 0.00% <0.00%> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/github.go 0.00% <0.00%> (ø)
modules/actions/workflows.go 0.00% <0.00%> (ø)
... and 62 more

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

@appleboy
Copy link
Member Author

@zeripath @wolfogre Done. Please help to review

models/actions/schedule.go Outdated Show resolved Hide resolved
models/actions/schedule.go Outdated Show resolved Hide resolved
models/actions/schedule_spec.go Outdated Show resolved Hide resolved
}

func startTasks(ctx context.Context, opts actions_model.FindSpecOptions) error {
specs, count, err := actions_model.FindSpecs(ctx, opts)
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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/notifier_helper.go Outdated Show resolved Hide resolved
}

func startTasks(ctx context.Context, opts actions_model.FindSpecOptions) error {
specs, count, err := actions_model.FindSpecs(ctx, opts)
Copy link
Member

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.

services/cron/tasks_actions.go Outdated Show resolved Hide resolved
services/cron/tasks_actions.go Outdated Show resolved Hide resolved
services/actions/schedule_tasks.go Outdated Show resolved Hide resolved
services/actions/schedule_tasks.go Outdated Show resolved Hide resolved
services/actions/schedule_tasks.go Outdated Show resolved Hide resolved
models/actions/schedule.go Outdated Show resolved Hide resolved
@yardenshoham yardenshoham added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/enhancement An improvement of existing functionality labels Mar 18, 2023
@lunny
Copy link
Member

lunny commented Mar 24, 2023

I think this could be refactored after https://gitea.com/gitea/act/pulls/29 merged.

@appleboy
Copy link
Member Author

All Done. Please help to review @wolfogre @lunny

modules/actions/workflows.go Outdated Show resolved Hide resolved
@techknowlogick
Copy link
Member

I've just resolved the conflicts

@techknowlogick techknowlogick changed the title chore(actions): support cron schedule task. chore(actions): support cron schedule task Apr 17, 2023
@delvh
Copy link
Member

delvh commented Jun 12, 2023

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

@silverwind silverwind added this to the 1.21.0 milestone Jun 12, 2023
wolfogre added a commit that referenced this pull request Jul 25, 2023
…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>
@BrunoBernardino
Copy link

BrunoBernardino commented Jul 25, 2023

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 !

@puni9869
Copy link
Member

Could you add some unit test cases.

@denyskon
Copy link
Member

denyskon commented Aug 1, 2023

@appleboy The merged auto-cancellation is only for push events, not for cron, so it won't help us here - or am I wrong?

@douglasparker
Copy link

Will this make it into gitea soon, or should I look into woodpecker ci or similar?

@silverwind
Copy link
Member

Happy to rubber-stamp this once the merge conflicts and CI failures are fixed.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 13, 2023
@denyskon
Copy link
Member

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?

e.Limit(opts.PageSize, (opts.Page-1)*opts.PageSize)
}
var specs SpecList
total, err := e.Desc("id").FindAndCount(&specs)
Copy link
Contributor

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"

@appleboy appleboy changed the title chore(actions): support cron schedule task WIP: chore(actions): support cron schedule task Aug 13, 2023
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
Copy link
Contributor

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?

Copy link
Member

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

@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 22, 2023
@lunny lunny closed this Aug 22, 2023
@GiteaBot GiteaBot removed this from the 1.21.0 milestone Aug 22, 2023
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 22, 2023
@lunny
Copy link
Member

lunny commented Aug 22, 2023

Sorry, I made a mistake. Please follow #26655

lunny added a commit that referenced this pull request Aug 24, 2023
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>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. topic/gitea-actions related to the actions of Gitea type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet