Fix CronMixin in task-sdk not resolving cron presets before validation#66102
Fix CronMixin in task-sdk not resolving cron presets before validation#66102shashbha14 wants to merge 8 commits into
Conversation
kaxil
left a comment
kaxil
left a comment
There was a problem hiding this comment.
Left some minor comments, but lgtm otherwise
kaxil
added
the
backport-to-v3-2-test
|
@shashbha14 — There are 4 unresolved review thread(s) on this PR from @kaxil. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
…e, add timetable test
|
@kaxil the only failing check is the MongoDB provider compatibility test ,all 33 errors are 'Timeout >60.0s' from testcontainers failing to pull the MongoDB Docker image. This has failed identically on 3 consecutive runs and |
| # Mirrors airflow.utils.dates.cron_presets in airflow-core (the SDK cannot | ||
| # import core). If a preset is added or changed here it must also be updated | ||
| # there, and vice-versa. | ||
| CRON_PRESETS: dict[str, str] = { | ||
| "@hourly": "0 * * * *", | ||
| "@daily": "0 0 * * *", | ||
| "@weekly": "0 0 * * 0", | ||
| "@monthly": "0 0 1 * *", | ||
| "@quarterly": "0 0 1 */3 *", | ||
| "@yearly": "0 0 1 1 *", | ||
| } | ||
|
|
There was a problem hiding this comment.
This needs a way to keep things in sync automatically. A pre-commit hook maybe.
There was a problem hiding this comment.
Instead of a pre-commit hook, made airflow-core re-export directly:
'from airflow.sdk.definitions.timetables._cron import CRON_PRESETS as cron_presets' in 'airflow/utils/dates.py'. The SDK is now the single source of truth ,no duplication, no sync needed.
|
@shashbha14 can you fix static checks |
Merge branch 'fix/cron-mixin-presets-66101' of https://github.com/shashbha14/airflow into fix/cron-mixin-presets-66101
Fixed — the static check was failing because the re-export approach |
5 participants
Closes #66101
The problem was Cron presets like '@quarterly' ceased functioning in Airflow 3.2.1.Upon moving 'CronMixin' to the task-sdk module, the process to resolve the preset values disappeared. With the introduction of the @attrs.define
decorator, the preset value is passed straight to 'croniter', where @quarterly isn't recognised, resulting in 'CroniterBadCronError', raised as 'AirflowTimetableInvalid'.
Fix-
Included a 'CRON_PRESETS' dictionary in the _cron.py module of the SDK and added an 'attrs_post_init' function that would convert the preset name to its corresponding cron expression before performing any further
validation or scheduling operations.