woodpecker icon indicating copy to clipboard operation
woodpecker copied to clipboard

WIP: Add cron jobs

Open 6543 opened this issue 2 years ago • 2 comments

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

6543 avatar May 20 '22 11:05 6543

Related to #8

mscherer avatar May 20 '22 16:05 mscherer

Codecov Report

Merging #934 (358384f) into master (08a9915) will decrease coverage by 1.02%. The diff coverage is 16.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.

codecov-commenter avatar Jun 02 '22 01:06 codecov-commenter

image

:tada:

6543 avatar Aug 25 '22 19:08 6543

ok we need to decide ...

for cron jobs, what user thumbnail we should show! image A. nothing as atm B. repo owner C. cronjob creator D. a new icon spec for cronjobs

6543 avatar Aug 25 '22 19:08 6543

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.

UnlimitedCookies avatar Aug 26 '22 01:08 UnlimitedCookies

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.

anbraten avatar Aug 27 '22 14:08 anbraten

For timezones could we just assume the cron syntax is in UTC and add a note that its UTC for now?

anbraten avatar Aug 27 '22 14:08 anbraten

Screenshot from 2022-08-27 23-52-21 Screenshot from 2022-08-27 23-38-02

anbraten avatar Aug 27 '22 21:08 anbraten

currently tested at codeberg ... waiting for feedback

todo left: Docs ;)

6543 avatar Aug 27 '22 23:08 6543

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.

anbraten avatar Aug 28 '22 07:08 anbraten

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-934.surge.sh

woodpecker-bot avatar Aug 29 '22 20:08 woodpecker-bot

https://woodpecker-ci-woodpecker-pr-934.surge.sh/docs/usage/cron
https://woodpecker-ci-woodpecker-pr-934.surge.sh/docs/usage/pipeline-syntax#cron

6543 avatar Aug 29 '22 20:08 6543

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 add when.event: cron to all pipeline steps which should be executed by a cron job

6543 avatar Aug 29 '22 20:08 6543

split refactoring out: #1141

6543 avatar Aug 29 '22 22:08 6543

  • All events (including cron jobs) will trigger all steps without a event filter

@6543 I think this is much more intuitive.

kdumontnu avatar Aug 30 '22 13:08 kdumontnu

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

anbraten avatar Aug 30 '22 13:08 anbraten

  • 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 set when.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 avatar Aug 30 '22 14:08 kdumontnu

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

anbraten avatar Aug 30 '22 14:08 anbraten

anything else blocking it?

else we should itterate on this via new pulls

6543 avatar Aug 30 '22 23:08 6543