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

Migrate to maintained cron lib and remove seconds #3785

Merged
merged 10 commits into from
Aug 6, 2024

Conversation

qwerty287
Copy link
Contributor

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

@qwerty287 qwerty287 added refactor delete or replace old code breaking will break existing installations if no manual action happens labels Jun 13, 2024
@zc-devs
Copy link
Contributor

zc-devs commented Jun 13, 2024

If you build an image, I can test.

@qwerty287 qwerty287 added the build_pr_images If set, the CI will build images for this PR and push to Dockerhub label Jun 13, 2024
@qwerty287
Copy link
Contributor Author

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
Copy link
Collaborator

woodpecker-bot commented Jun 13, 2024

Deployment of preview was torn down

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 21.05263% with 15 lines in your changes missing coverage. Please review.

Project coverage is 26.89%. Comparing base (123c4ae) to head (dc125cf).
Report is 5 commits behind head on main.

Files Patch % Lines
.../store/datastore/migration/033_cron_without_sec.go 17.64% 11 Missing and 3 partials ⚠️
server/model/cron.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3785      +/-   ##
==========================================
- Coverage   26.89%   26.89%   -0.01%     
==========================================
  Files         394      395       +1     
  Lines       27467    27484      +17     
==========================================
+ Hits         7388     7391       +3     
- Misses      19380    19391      +11     
- Partials      699      702       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xoxys
Copy link
Member

xoxys commented Jun 13, 2024

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

@qwerty287
Copy link
Contributor Author

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
Copy link
Member

anbraten commented Jun 13, 2024

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

@xoxys
Copy link
Member

xoxys commented Jun 14, 2024

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
Copy link
Member

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
Copy link
Contributor

pat-s commented Jun 15, 2024

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

@qwerty287 qwerty287 added this to the 3.0.0 milestone Jul 17, 2024
@6543 6543 mentioned this pull request Jul 19, 2024
Copy link
Contributor

@pat-s pat-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrating to a maintained lib is certainly a good idea - in addition to removing seconds (which are of no practical use anyway).

@pat-s pat-s enabled auto-merge (squash) August 6, 2024 17:02
@pat-s pat-s merged commit c864f24 into woodpecker-ci:main Aug 6, 2024
7 checks passed
@qwerty287 qwerty287 deleted the cron-lib branch August 6, 2024 17:26
@woodpecker-bot woodpecker-bot mentioned this pull request Aug 6, 2024
1 task
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
Co-authored-by: Patrick Schratz <patrick.schratz@gmail.com>
@woodpecker-bot woodpecker-bot mentioned this pull request Sep 8, 2024
1 task
@anbraten anbraten mentioned this pull request Nov 22, 2024
4 tasks
@woodpecker-bot woodpecker-bot mentioned this pull request Dec 14, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking will break existing installations if no manual action happens build_pr_images If set, the CI will build images for this PR and push to Dockerhub dependencies refactor delete or replace old code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants