woodpecker
woodpecker copied to clipboard
WIP: Add cron jobs
TODOs
- [ ] handle timezones (https://blog.fuzzymistborn.com/drone-love-hate/)
- [x] use https://github.com/robfig/cron to parse next schedule
- [x] #949 got merged
- [x] add api
- [ ] add cli subcomand
- [x] add ui
- [ ] add docs
- [ ] tests ...
- [ ] integration tests
- [ ] ... https://docs.drone.io/pipeline/docker/syntax/trigger/#by-cron
close #8
Related to #8
Codecov Report
Merging #934 (358384f) into master (08a9915) will decrease coverage by
1.02%
. The diff coverage is16.41%
.
@@ Coverage Diff @@
## master #934 +/- ##
==========================================
- Coverage 50.40% 49.38% -1.03%
==========================================
Files 83 86 +3
Lines 6341 6535 +194
==========================================
+ Hits 3196 3227 +31
- Misses 2959 3120 +161
- Partials 186 188 +2
Impacted Files | Coverage Δ | |
---|---|---|
server/model/build.go | 0.00% <ø> (ø) |
|
server/model/cron.go | 0.00% <0.00%> (ø) |
|
server/remote/bitbucket/bitbucket.go | 85.71% <0.00%> (-1.17%) |
:arrow_down: |
server/remote/bitbucketserver/bitbucketserver.go | 0.00% <0.00%> (ø) |
|
server/remote/coding/coding.go | 62.96% <0.00%> (-0.89%) |
:arrow_down: |
server/remote/gitea/gitea.go | 34.27% <0.00%> (-1.42%) |
:arrow_down: |
server/remote/github/github.go | 15.30% <0.00%> (-0.42%) |
:arrow_down: |
server/remote/gitlab/gitlab.go | 22.43% <0.00%> (-0.88%) |
:arrow_down: |
server/remote/gogs/gogs.go | 50.23% <0.00%> (-2.50%) |
:arrow_down: |
server/store/datastore/migration/migration.go | 37.50% <ø> (ø) |
|
... and 4 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
:tada:
ok we need to decide ...
for cron jobs, what user thumbnail we should show!
A. nothing as atm
B. repo owner
C. cronjob creator
D. a new icon spec for cronjobs
A. nothing as atm B. repo owner C. cronjob creator D. a new icon spec for cronjobs
Both B and C would be confusing. I would just show a stopwatch icon in place of the user avatar.
As for the timezones, those should mostly be a concern of the frontend. Maybe there can be a dropdown with all timezones to choose from and then while saving the offset is calculated and just the local time is inserted into the db. For the time input, I think most Linux geeks like me would be fine with just a text input box to enter 1 2 * * *
with perhaps some help like “At 14:15 on day-of-month 1.”
as it can be found on https://crontab.guru/. But all of these things are nice to haves and shouldn't delay a first release of the cron feature.
I would also suggest to just have an empty string in the icon data for pipelines executed by crontabs and show some stopwatch etc in the frontend in the case the event type is a cronjob.
For timezones could we just assume the cron syntax is in UTC and add a note that its UTC for now?
currently tested at codeberg ... waiting for feedback
todo left: Docs ;)
I am not sure if I like the way that cron pipelines only pick steps with when.event: cron. I would expect them to execute each step as usual and only skip jobs with unmatched filters.
Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-934.surge.sh
https://woodpecker-ci-woodpecker-pr-934.surge.sh/docs/usage/cron
https://woodpecker-ci-woodpecker-pr-934.surge.sh/docs/usage/pipeline-syntax#cron
One discussion to take place before we can merge:
Should we add a "default value / list" for the event filter. There are two possibilities:
- All events (including cron jobs) will trigger all steps without a event filter
- The event filter will get a default list of events
tag, push, pull_request, deploy
which are applied if a step has no specific event filter. This way you need to explicitly addwhen.event: cron
to all pipeline steps which should be executed by a cron job
split refactoring out: #1141
- All events (including cron jobs) will trigger all steps without a event filter
@6543 I think this is much more intuitive.
- All events (including cron jobs) will trigger all steps without a event filter
@6543 I think this is much more intuitive.
I am still not 100% convinced by default event filters for the same reason, but at the moment it seems that most members are preferring a default filter.
- All events (including cron jobs) will trigger all steps without a event filter
@6543 I think this is much more intuitive.
I am still not 100% convinced by default event filters for the same reason, but at the moment it seems that most members are preferring a default filter.
😮 That's surprising. I think it will open up many more questions/issues for users. I guess I'll make my case here:
- Having a default set of values for "filter" makes it not really a filter, but actually more like a "mode". That is, not setting a filter should imply that everything will pass through (i.e. no filter!). Setting a value for filter implies you are limiting some data to pass through.
- A (probably THE) typical use-case is to create a separate pipeline for cron jobs. Users will have to not only set a
trigger
for cron, but then also setwhen.event: cron
for every step.
That being said, I support whatever decision you all go with - this shouldn't hold up the feature from introduction. It sounds great either way.
@kdumontnu Thanks for this nice exploration. I can totally agree. In addition I guess most users will have less problems first running more steps then wanted by a cron pipeline and will simply add filters as users will be confused why a cron job triggered no steps until they ask in the chat or read the docs. What I am also not quite sure about is how it will work when introducing the root level when filter as you would need to "allow" steps to be executed on the step and root level then.
Anyways we already added the default logic with #1140. Before we do the next stable release we can just test it and simply do a breaking change by reverting it again if needed.
anything else blocking it?
else we should itterate on this via new pulls