Skip to content

refactor!: Refactor cronjob controller to reduce duplication #3421

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

Merged
merged 16 commits into from
Jun 6, 2025

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented May 26, 2025

The cronjob controller contained a lot of essentially duplicated logic after the addition of background events. To resolve this, I've refactored the cronjob controller to treat cronjobs as recurring background events. This means:

  • Cronjobs and background events are now (re)scheduled and executed using the same logic.
  • Cronjobs are stored in state just like background events.
    • I removed the lastRun property in favour of a date property. We can now simply check if the current date is past the scheduled date to know if a cronjob should be executed immediately.
    • This simplifies the logic for rescheduling cronjobs significantly.
  • Cronjobs can now use ISO 8601 durations as well as the usual cron expression.
  • The logic for getting all cronjobs from all Snaps was removed, since the full cronjobs are stored in state now.
    • This works fine for new Snaps, but I need to consider how to handle existing Snaps at the time of the cronjob controller update, either with a migration or by re-adding some of this logic.

In addition to these changes, I've made some smaller changes to the controller like replacing TypeScript's private modifier with true hash private functions.

Breaking changes

  • The jobs state property was removed, cronjobs are now stored in the events property as well.
  • CronjobController:schedule now expects a schedule field instead of date.
  • Some actions were renamed to remove the BackgroundEvent suffix.
    • CronjobController:scheduleBackgroundEvent -> CronjobController:schedule
    • CronjobController:cancelBackgroundEvent -> CronjobController:cancel.
    • CronjobController:getBackgroundEvents -> CronjobController:get.

@Mrtenz Mrtenz changed the title Refactor cronjob controller to reduce duplication refactor: Refactor cronjob controller to reduce duplication May 26, 2025
*/
function getStartDate(params: ScheduleBackgroundEventParams) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The logic for scheduling was previously done in snaps-rpc-methods, but handling this from the cronjob controller is easier to reduce duplication.

@@ -20,7 +20,7 @@
"endowment:cronjob": {
"jobs": [
{
"expression": "*/10 * * * * *",
"expression": "PT10S",
Copy link
Member

Choose a reason for hiding this comment

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

We need to find a way to test both formats 😓

Copy link
Member Author

@Mrtenz Mrtenz May 26, 2025

Choose a reason for hiding this comment

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

Likely easiest with a new example Snap. Just changed it here for local testing purposes.

@Mrtenz Mrtenz force-pushed the mrtenz/refactor-cronjob-controller branch from 794289e to 8951701 Compare May 26, 2025 11:41
Copy link

codecov bot commented May 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.18%. Comparing base (8854c9f) to head (dfb3b78).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3421      +/-   ##
==========================================
+ Coverage   98.16%   98.18%   +0.01%     
==========================================
  Files         402      399       -3     
  Lines       11141    11127      -14     
  Branches     1750     1750              
==========================================
- Hits        10937    10925      -12     
+ Misses        204      202       -2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Mrtenz Mrtenz marked this pull request as ready for review May 27, 2025 11:32
@Mrtenz Mrtenz requested a review from a team as a code owner May 27, 2025 11:32
@Mrtenz Mrtenz requested a review from FrederikBolding May 27, 2025 11:32
FrederikBolding
FrederikBolding previously approved these changes Jun 6, 2025
@FrederikBolding FrederikBolding changed the title refactor: Refactor cronjob controller to reduce duplication refactor!: Refactor cronjob controller to reduce duplication Jun 6, 2025
@Mrtenz Mrtenz force-pushed the mrtenz/refactor-cronjob-controller branch from 79d4de2 to c16a656 Compare June 6, 2025 14:08
FrederikBolding
FrederikBolding previously approved these changes Jun 6, 2025
@FrederikBolding
Copy link
Member

@Mrtenz Can you add a note about the breaking changes in the description please?

@Mrtenz Mrtenz added this pull request to the merge queue Jun 6, 2025
Merged via the queue into main with commit 02995f1 Jun 6, 2025
117 checks passed
@Mrtenz Mrtenz deleted the mrtenz/refactor-cronjob-controller branch June 6, 2025 15:02
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.

2 participants