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

Cron mode #19

Merged
merged 11 commits into from
Nov 19, 2023
Merged

Cron mode #19

merged 11 commits into from
Nov 19, 2023

Conversation

ErfanMomeniii
Copy link
Contributor

Hi,
Merging this pull request can rerun jobs in a particular interval of time.
If you have any ideas or concerns regarding the PR, feel free to tell me.

@liweiyi88
Copy link
Owner

Hi @ErfanMomeniii ,
May I ask if you have some valid use cases to embed cron-like features into this tool while ppl can use tools like crontab, k8s cronjob or Airplane schedules to re-run the tool periodically?

@ErfanMomeniii
Copy link
Contributor Author

I think this feature offers the advantage of eliminating the need for additional tools or concepts to be periodically re-run, thereby providing users with a convenient and optional solution.

@liweiyi88
Copy link
Owner

I am not convinced that it is practical to embed cron-like feature in onedump for prod operations. The main concern is that ppl will still need the external app to recover from the failure of running onedump (e.g. when OOM happened and onedump just crashed)

But I understand that there might be use cases ppl would like to experiment this tool and run it as cron job without external configuration.

Having said that, if we would like to support cron-like feature, lets do it in that way. My suggestion is to use package like https://github.com/jasonlvhit/gocron to support the cron configuration. 🙏

@ErfanMomeniii
Copy link
Contributor Author

Done, I also changed some arrangements of imports for having better styles.

@liweiyi88
Copy link
Owner

@ErfanMomeniii thanks so much for your contribution. Just 2 suggestion in order to merge the PR.

  1. It seems that the https://github.com/jasonlvhit/gocron project has been moved to https://github.com/go-co-op/gocron according to the README file, could we update the dependency and follow the latest practice?
  2. In order to have a bit better user experience, I would suggest renaming the interval flag to cron to allow users to pass something like --cron 5m, Also allow users to pass a valid cron expression like --cron */1 * * * * * (which I saw the latest package support then out of the box, but you probably need to add logic to distinguish a valid cron expression or a normal string expression). I am not too sure if two options like --every 5m and --cron */1 * * * * * make more sense than a single --cron option at the moment.
    Let me know if you are not interested in making those changes, then I will merge your PR and implement the changes myself.

@ErfanMomeniii
Copy link
Contributor Author

I changed the interval flag to --cron and used the new package instead of the previous one to add a cron job.

@liweiyi88
Copy link
Owner

@ErfanMomeniii , nice work and thanks you 🎉

@liweiyi88
Copy link
Owner

@ErfanMomeniii btw, could you fix the pipeline error when you get a chance?

@ErfanMomeniii
Copy link
Contributor Author

@liweiyi88, it should be fixed now.

@liweiyi88
Copy link
Owner

I have also put a few minor comments in the PR

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2023

Codecov Report

Patch coverage: 20.00% and project coverage change: -0.89% ⚠️

Comparison is base (02b20ed) 76.23% compared to head (2b68644) 75.35%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
- Coverage   76.23%   75.35%   -0.89%     
==========================================
  Files          19       19              
  Lines         968      982      +14     
==========================================
+ Hits          738      740       +2     
- Misses        159      170      +11     
- Partials       71       72       +1     
Files Changed Coverage Δ
config/job.go 100.00% <ø> (ø)
driver/mysql.go 67.74% <ø> (ø)
driver/postgresql.go 76.56% <ø> (ø)
dumper/sshdumper.go 58.20% <ø> (ø)
handler/dumphandler.go 80.35% <ø> (ø)
handler/jobhandler.go 75.15% <ø> (ø)
storage/gdrive/gdrive.go 83.33% <ø> (ø)
storage/s3/s3.go 92.85% <ø> (ø)
cmd/root.go 50.00% <20.00%> (-13.16%) ⬇️

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

@ErfanMomeniii
Copy link
Contributor Author

ErfanMomeniii commented Jul 22, 2023

I'll add some tests for increasing test coverage for each part of the project and notify you for checking that.
Thanks @liweiyi88

@liweiyi88
Copy link
Owner

Thank you @ErfanMomeniii

@liweiyi88 liweiyi88 merged commit d98e339 into liweiyi88:main Nov 19, 2023
@liweiyi88 liweiyi88 changed the title Adding interval flag for setting amount of the times to rerun jobs Cron mode Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants