refactor(core): Centralize CronJob management #10033
Conversation
n8n-assistant
bot
added
core
netroy
force-pushed
the
global-registry-cron
branch
from
bf4ed2e
to
55bd8b4
Compare
netroy
force-pushed
the
global-registry-cron
branch
from
55bd8b4
to
e387cec
Compare
packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts
Outdated
Show resolved
Hide resolved
netroy
force-pushed
the
global-registry-cron
branch
from
014ec37
to
7a49bd7
Compare
| }); | ||
| }); | ||
|
|
||
| describe.only('recurrenceCheck', () => { |
4 flaky tests on run #5961 ↗︎
Details:
|
||||||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Undo/Redo > should undo/redo adding connected nodes |
Test Replay
Screenshots
Video
|
|
5-ndv.cy.ts • 2 flaky tests
| Test | Artifacts | |
|---|---|---|
| NDV > should not retrieve remote options when required params throw errors |
Screenshots
Video
|
|
| NDV > Stop listening for trigger event from NDV |
Screenshots
Video
|
|
24-ndv-paired-item.cy.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| NDV > resolves expression with default item when input node is not parent, while still pairing items |
Test Replay
Screenshots
Video
|
|
Review all test suite changes for PR #10033 ↗︎
|
✅ All Cypress E2E specs passed |
…hour" crons from all triggering at the same minute
| } | ||
| const fallbackToZero = addFallbackValue(nodeVersion >= 1.2, '0'); |
There was a problem hiding this comment.
This was introduced to fix a behavior for schedule nodes version 1.2 and above.
For version 1.0 and 1.1 if you set triggerAtMinute to an empty string it will run every minute, as opposed to every hour/day/week, etc.
This was fixed here: #9146
The fix added the version 1.2 and only applied to that and following versions. I'm assuming this was done so that it would not break users that rely on the buggy behavior.
With this refactor we apply the "fixed" behavior to all versions now. Which is technically a breaking change.
@Joffcom sorry for looping you in here out of the blue, but maybe you have some insight if we can assume that this will break some workflows in the wild or if we can just "fix" it for all versions?
There was a problem hiding this comment.
@despairblue correct, changes that could be breaking are usually scoped under version, if it's not bugfix
we need to update packages/cli/BREAKING-CHANGES.md in this case
There was a problem hiding this comment.
I'm gonna try to add the check back, and also add tests to prevent this from breaking in the future 🤞🏽
There was a problem hiding this comment.
For version 1.0 and 1.1 if you set triggerAtMinute to an empty string it will run every minute, as opposed to every hour/day/week, etc.
When I try this on older versions of the node, the Cron expression that's generated is ' * * * *', which in an invalid expression.
@michael-radency do you recall why did we decide to support this old behavior?
IMO, this is bug, and shouldn't have been supported. All versions of this node should fallback to 0 when the field is an empty string.
There was a problem hiding this comment.
In the new code now if a user leaves the minute empty, we generate a valid cron expression by falling back to a random minute during activation.
I'm still not sure why we created a new version in #9146.
I believe this was a bug, and we should have fixed this in the existing version of the node.
If user is adding invalid values in any of the fields, then we don't owe them backward compatibility, nor do we need to document this as a breaking change.
IMO the changes proposed in this PR are backward compatible for anyone using this node as intended, for everyone else when the minute field is left invalid, falling back to selecting a random minute during activation works fine.
|
|
|
✅ All Cypress E2E specs passed |
|
Got released with |
Summary
Letting individual nodes manage CronJob
This PR aims to centralize how we register cronJobs, to have a better control over their lifecycle.
Review / Merge checklist