search

woodpecker-ci / woodpecker

Woodpecker is a simple yet powerful CI/CD engine with great extensibility.
https://woodpecker-ci.org
Apache License 2.0
3.88k stars 345 forks source link

Migrate to maintained cron lib and remove seconds #3785

Open qwerty287 opened 3 weeks ago

qwerty287 commented 3 weeks ago

Use https://github.com/gdgvda/cron and remove seconds from cron syntax to use standard cron.

zc-devs commented 3 weeks ago

If you build an image, I can test.

qwerty287 commented 3 weeks ago

Started CI, wait a few minutes.

Note that I wrote a migration that will remove the seconds, but if you revert back you have to update all crons manually and add the seconds back

woodpecker-bot commented 3 weeks ago

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

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 25.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 25.81%. Comparing base (17b4b81) to head (fff0aa9). Report is 2 commits behind head on main.

:exclamation: Current head fff0aa9 differs from pull request most recent head fe19634

Please upload reports for the commit fe19634 to get more accurate results.

Files Patch % Lines
.../store/datastore/migration/031_cron_without_sec.go 21.42% 8 Missing and 3 partials :warning:
server/model/cron.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3785 +/- ## ========================================== - Coverage 28.06% 25.81% -2.26% ========================================== Files 363 363 Lines 25108 26739 +1631 ========================================== - Hits 7047 6902 -145 - Misses 17536 19301 +1765 - Partials 525 536 +11 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

xoxys commented 3 weeks ago

What about https://github.com/go-co-op/gocron? The one from gdgvda/crongdgvda/cron doesn't look that reliable as well.

qwerty287 commented 3 weeks ago

What do you mean by "reliable"?

I also saw that but preferred the fork because it was much easier to migrate. But I can also use the gocron lib.

Besides that, the "reliability" is not that important actually. This is done internally - the libs are only used to determine the time of the next run.

anbraten commented 3 weeks ago

https://github.com/go-co-op/gocron is using https://github.com/robfig/cron internally

xoxys commented 3 weeks ago

What do you mean by "reliable"?

My main concern was the reputation of the owner/org. The repo has not many stars, the maintainer has nearly no activity on GitHub, same applies to the org.

If we just use the Parser, why not embed this part in the WP code directly? The parser code wasn't touched since 4 years, even in the new fork. However, I don't have a strong opinion on this and won't block you, I'm just a bit more careful about unpopular repos/authors.

anbraten commented 3 weeks ago

If we just use the Parser, why not embed this part in the WP code directly? The parser code wasn't touched since 4 years, even in the new fork. However, I don't have a strong opinion on this and won't block you, I'm just a bit more careful about unpopular repos/authors.

We could also just stick to the lib as it works AFAIK and just change the format to the one without seconds.

pat-s commented 3 weeks ago

If we would remove seconds, this would be a breaking change and should go into 3.0.