kiorky / croniter

MIT License
403 stars 39 forks source link

Latest version is not working as expected - fails loading a proper cron_schedule string #47

Closed davidalbertonogueira closed 1 year ago

davidalbertonogueira commented 1 year ago

Latest version is not working as expected. Fails loading a proper cron_schedule string:


  File "/opt/dagster/dagster_home/<PROJECT_NAME>/schedules/stores_schedule.py", line 13, in <module>
    @schedule(name='stores_schedule', job=SCHEDULED_JOB, cron_schedule="0 0 * * 6", execution_timezone=run_config.constants.EXECUTION_TIMEZONE)
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/dagster_env/lib/python3.11/site-packages/dagster/_core/definitions/decorators/schedule_decorator.py", line 174, in inner
    schedule_def = ScheduleDefinition(
                   ^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/dagster_env/lib/python3.11/site-packages/dagster/_core/definitions/schedule_definition.py", line 538, in __init__
    if not is_valid_cron_schedule(self._cron_schedule):  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/conda/envs/dagster_env/lib/python3.11/site-packages/dagster/_utils/schedules.py", line 22, in is_valid_cron_schedule
    is_valid_cron_string(cron_schedule)
  File "/opt/conda/envs/dagster_env/lib/python3.11/site-packages/dagster/_utils/schedules.py", line 15, in is_valid_cron_string
    expanded, _ = croniter.expand(cron_string)
    ^^^^^^^^^^^
eduardev commented 1 year ago

Same issue as above, although problem is actually Dagster not pinning dependencies. Nonetheless, not sure if cronitor authors wanted to introduce a breaking change on a minor version.

Dagster related issue https://github.com/dagster-io/dagster/pull/14811

kiorky commented 1 year ago

authors wanted to introduce a breaking change on a minor version.

ATTENTION !!! It's not minor !!! 3 -> 4, and changelog list the BREAKING CHANGE

kiorky commented 1 year ago

So sorry, i mis read the mail, and though it was first a regression. Please note that the change is wanted, it's a breaking change, the version numbering broke to notice downstreams about it, and the aforementioned breaking change is listed in the changelog.

kiorky commented 1 year ago

Please hold, i ll try to make a more retrocompatible way of implementing #44, just pin to 1.3.16, and i ll make a fix in the hour...

kiorky commented 1 year ago

1.4.1 is out guys.

eduardev commented 1 year ago

@kiorky

  1. thank you for the quick turnaround, much appreciated
  2. according to semantic versioning, x.y.z the Y is actually a minor, should not have breaking changes
  3. there was indeed a changelog and breaking change was stated there, so, my bad nonetheless

Again, thank you!