Page MenuHomePhabricator

Add additional validation for start date and end date timezones
Needs RevisionPublic

Authored by dgibson on Tue, Jan 5, 8:24 PM.



Based on rex's good suggestion here:

Be more aggressive about timezone validation - if you set one, and you set a timezone on your start time or end time, it should match up with the execution timezone. Otherwise it's highly likely that your schedule will not be starting or ending where you expect.

Test Plan


Diff Detail

R1 dagster
rethinktimezonewarn (branched from master)
Lint OK
No Unit Test Coverage

Event Timeline




I'm confused... why do start/end time have to be a pendulum instance? I thought that pendulum was just a helper library....

Couldn't you do the same checks on a normal datetime object?


Discussed this off-thread, I guess other tz libraries don't handle DST very well.

would consider downgrading the pendulum instance error check to a warning, but this seems fine

This revision is now accepted and ready to land.Wed, Jan 6, 5:24 PM

the problem is that there's no way to validate or verify the timezone name if they're using some other tzinfo - the tzinfo spec is... quite bad.

"" ("Nothing about string names is defined by the datetime module, and there’s no requirement that it mean anything in particular. For example, “GMT”, “UTC”, “-500”, “-5:00”, “EDT”, “US/Eastern”, “America/New York” are all valid replies")

OK, I ended up rethinking this a bit, rather than being very strict about exactly what comes in, transform it into a Pendulum timezone

I think this is in a slightly confusing state, I would guess due to how it started vs the latest approach. Sending to you for one more spin for some mix of more tests or renaming / rewording


will this fail in any condition? coerce_schedule_boundary may be a more appropriate name


given the title of the diff I would expect additional tests for validation of start_date and end_date, but it seems we're now just doing a coercion so maybe update title?

should there still be tests of the start_date/end_date coercion?

This revision now requires changes to proceed.Mon, Jan 11, 6:08 PM