Skip to content

Conversation

@hussein-awala
Copy link
Member

closes: #29576


Currently, when we provide a datetime with timezone different from UTC to DateTimeTrigger, it raises an exception. Instead, I convert the datetime to UTC.

@hussein-awala hussein-awala marked this pull request as ready for review February 18, 2023 22:52
Comment on lines -44 to +45
elif not hasattr(moment.tzinfo, "offset") or moment.tzinfo.offset != 0: # type: ignore
raise ValueError(f"The passed datetime must be using Pendulum's UTC, not {moment.tzinfo!r}")
else:
self.moment = moment
self.moment = timezone.convert_to_utc(moment)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Tests Police" here 🤣 , we need also test this one.

A guess we do not have unittests for this constructor except type checking.
The same valid for TimeDeltaTrigger which based on DateTimeTrigger

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the test test_datetime_trigger_timing to check if the logic remains valid with different timezones

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could check just in class constructor but this seems like even better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taragolis looks like there are tests added. Do you think there should be more testing?

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait till CI green

Comment on lines -44 to +45
elif not hasattr(moment.tzinfo, "offset") or moment.tzinfo.offset != 0: # type: ignore
raise ValueError(f"The passed datetime must be using Pendulum's UTC, not {moment.tzinfo!r}")
else:
self.moment = moment
self.moment = timezone.convert_to_utc(moment)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could check just in class constructor but this seems like even better

@Taragolis Taragolis merged commit 79c07e3 into apache:main Feb 19, 2023
@pierrejeambrun pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Feb 27, 2023
@pierrejeambrun pierrejeambrun added this to the Airflow 2.5.2 milestone Feb 27, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
…29606)

* convert moments with timezone to utc instead of raising an exception

* test DateTimeTrigger with different timezones

(cherry picked from commit 79c07e3)
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
…29606)

* convert moments with timezone to utc instead of raising an exception

* test DateTimeTrigger with different timezones

(cherry picked from commit 79c07e3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DateTimeSensorAsync breaks if target_time is timezone-aware

4 participants