register all cron jobs in entrypoint #12791
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added centralized cron job registration functionality in the server entrypoint, with new command and module exports to manage background sync jobs.
- Command placement in
database/commandsis architecturally misaligned; consider moving to a dedicatedcronorschedulermodule given its distinct responsibility - Missing environment variable control (like
DISABLE_CRON_JOBS_REGISTRATION) for different deployment scenarios - Error handling in
entrypoint.shcould be improved to include specific error logging and proper cleanup of stale jobs - Consider race conditions where cron jobs might be registered before the database is fully ready
- Added exports in multiple modules (
MessagingImportManagerModule,CalendarEventImportManagerModule) should be documented for better maintainability
7 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
| register_background_jobs() { | ||
|
|
||
| echo "Registering background sync jobs..." | ||
| if yarn command:prod cron:register:all; then | ||
| echo "Successfully registered all background sync jobs!" | ||
| else | ||
| echo "Warning: Failed to register background jobs, but continuing startup..." | ||
| fi | ||
| } |
There was a problem hiding this comment.
style: Similar to DISABLE_DB_MIGRATIONS, consider adding DISABLE_CRON_JOBS flag to control job registration behavior
There was a problem hiding this comment.
much better name than what I suggested in description :)
There was a problem hiding this comment.
I actually prefer your name DISABLE_CRON_JOBS_REGISTRATION. And I think it would be a good idea to add this so that we can disable it in our cloud yes!
| @@ -2,3 +2,4 @@ | |||
| .env | |||
| node_modules | |||
| .nx/cache | |||
| packages/twenty-server/.env | |||
There was a problem hiding this comment.
I took too long to figure this out but apparently the existing .env rule only ignores root-level files packages/twenty-server/.env was still being copied with localhost:5432, breaking container db connections
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:6612 This environment will automatically shut down when the PR is closed or after 5 hours. |
| } | ||
|
|
||
| async run(): Promise<void> { | ||
| this.logger.log('Registering all background sync cron jobs...'); |
There was a problem hiding this comment.
In a couple weeks let's update the docs to emphasize on the admin panel to check everything is properly started and remove the part that tells the user to run this manually
|
Thanks @ehconitin for your contribution! |
closes twentyhq/core-team-issues#1113
Three things I am not sure about -