Skip to content
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

Additional cron expression validation #16731

Open
uranusjr opened this issue Jun 30, 2021 · 3 comments
Open

Additional cron expression validation #16731

uranusjr opened this issue Jun 30, 2021 · 3 comments
Labels

Comments

@uranusjr
Copy link
Member

uranusjr commented Jun 30, 2021

Description
Airflow should outline (in documentation) what cron expressions are supported, and perform appropriate additional validation on user-supplied cron expressions.

Use case / motivation
#16692 proposed displaying a short description about the cron expression in the web UI. This bring out the issue that currently the cron expression formats supported by Airflow are essentially defined by croniter implementation, which is non-standard (POSIX only allows five-segment formats) and sometimes buggy (see #16107 (comment)). Since croniter itself does not offer expression description, we need to do this with another tool, meaning that non-standard expressions can be incorrectly described and cause user confusion.

It would be beneficial for Airflow to explicitly specify what cron expression formats are supported (only POSIX, or POSIX plus extensions like the optional year segment, or something else), and perform additional validation to ensure croniter does not receive formats not supported by Airflow, even if it can handle them. Personally I’m inclined to only do POSIX and reject the sixth segment entirely, at least until croniter fixes its handling to correctly interpret it (see taichino/croniter#76).

Are you willing to submit a PR?
Yeah

Related Issues
#16107 (comment)
#16692
taichino/croniter#76

@uranusjr uranusjr added the kind:feature Feature Requests label Jun 30, 2021
@eladkal
Copy link
Contributor

eladkal commented Jun 30, 2021

By addition validation do you mean to remove the 6 segmant and then trust croniter ?

@uranusjr
Copy link
Member Author

uranusjr commented Jun 30, 2021

I think we should build a simple validator to check for the whole string before passing it into croniter (and hopefully croniter will always work if the string passes our test).

The standard POSIX syntax is fairly easy to parse, plus support for SUN-SAT. Is JAN-DEC a GNU extension? That can be added as well if it works in croniter.

@ashb
Copy link
Member

ashb commented Jul 1, 2021

We don't want to restrict to just POSIX cron -- some of the non-POSIX extensions are widely supported and useful 0 0 * * 5#3,L5 for instace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants