Feat: accept cron schedule for analyzers #56
Conversation
murilommen
had a problem deploying
to
whylabs-toolkit-ci
GitHub Actions
Failure
murilommen
force-pushed
the
dev/murilommen/feat-add-cron-schedule
branch
from
9dc48a3 to
2529259
Compare
murilommen
had a problem deploying
to
whylabs-toolkit-ci
GitHub Actions
Failure
murilommen
temporarily deployed
to
whylabs-toolkit-ci
GitHub Actions
Inactive
murilommen
temporarily deployed
to
whylabs-toolkit-ci
GitHub Actions
Inactive
| def _is_not_less_granular_than_1_hour(split_cron: SplitCron) -> bool: | ||
| """Check if the cron expression is less granular than 1 hour.""" | ||
| # Specific days checks | ||
| if split_cron.minute != "*" and split_cron.minute != "0": |
There was a problem hiding this comment.
I dont get why it needs to check that its not 0 (and ditto for the other fields). Seems like this check should fail if the minute is '*' or anything with a - or , in it. Anything else would be ok.
There was a problem hiding this comment.
true. it really doesn't make sense to check for the "top of the {time}"
| if split_cron.day_of_week != "*" and split_cron.day_of_week != "1": | ||
| return True | ||
|
|
||
| # Check range |
There was a problem hiding this comment.
all that happens from here on is that it iterates and then returns False... logic seems wrong. Also seems disconnected from "is granularity less than hour", seems like it is meant to be checking any range fields
murilommen
had a problem deploying
to
whylabs-toolkit-ci
GitHub Actions
Failure
|
@christinedraper added a few changes that addresses these comments, see if they're enough to resolve the conversations 🙏🏽 and thanks for catching these |
details:
- include every N {field} on base regex on CronSchedule
- get rid of "strictly 0 {field}" cases, which didn't make sense
- fix order of SplitCron implementation
- checks for "," and "-" cases only on the minute field
- added tests
murilommen
force-pushed
the
dev/murilommen/feat-add-cron-schedule
branch
from
5576732 to
2ff300d
Compare
murilommen
temporarily deployed
to
whylabs-toolkit-ci
GitHub Actions
Inactive
It is used when creating CronSchedule and pydantic applies it |
Previously we had left out the ability to define an
Analyzer.schedule = CronSchedule. This PR adds that as a feature and also includes two guardrails for the cron string:I've used the
re.matchto capture invalid strings and also a unit test with a few different examples