-
Notifications
You must be signed in to change notification settings - Fork 15.9k
Fix cron expression display for Day-of-Month and Day-of-Week conflicts #54644
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
Conversation
|
Hi @uranusjr , can you please review the code changes. |
|
Hi @pierrejeambrun , @kaxil can you please review the changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I'm not in favor of this change. I don't really understand why this is needed. I would keep the standard cron description, if users cannot read cron expressions, they should go check the documentation. (I'm not strongly against it if this helps users)
Can you add relevant tests please.
Hi @pierrejeambrun , |
|
I mean if this is a more accurate description then sure. I also see the cron-descriptor has the same bug. Looks like we need to rebase and fix some tests though. |
The existing description is objectively incorrect. It's not just more accurate; the existing implementation is broken and the proposed fix makes it not broken. |
|
OK I get it now. Current default description is wrong when both are specified. (I tested on other tools and they all do the same mistake). Yes the tooltip needs to be fixed Is that the only known 'bug' in the cron description?
|
This is the only bug that I'm aware of. Other descriptions appear accurate. |
Glad to hear. I think we can move forward then. @shreyaskj-0710 Do you mind rebasing the PR and fixing the CI please. |
Hi @pierrejeambrun , Have updated the PR and fixed the CI |
…eek conflicts (#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- (cherry picked from commit c6531bb) Co-authored-by: shreyaskj-0710 <[email protected]> Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
…eek conflicts (#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- (cherry picked from commit c6531bb) Co-authored-by: shreyaskj-0710 <[email protected]> Co-authored-by: Ryan Hatter <[email protected]>
…eek conflicts (#54644) (#56255) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- (cherry picked from commit c6531bb) Co-authored-by: shreyaskj-0710 <[email protected]> Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
apache#54644) * Fix cron expression display for Day-of-Month and Day-of-Week conflicts * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute * Add test case for CronMixin description attribute --------- Co-authored-by: Ryan Hatter <[email protected]>
Fixes #54482
Description
📌 Day-of-Month (DOM) and Day-of-Week (DOW) Conflict in Cron
In cron syntax, both Day-of-Month (DOM) and Day-of-Week (DOW) fields are evaluated independently.
If both are specified, the schedule runs when either condition matches (logical OR), not when both are true together.
Example
Cron expression : * * */1 * 5
*/1 → effectively means every day of the month
Together → Every minute (or) Every minute, only on Friday (but since */1 means every day, the Friday part becomes
redundant).
Approach
Each variant is described independently, and the final description combines them using “(or)”, which matches actual cron semantics.
In UI Timetable description will be as follows
