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

convert moment with timezone to UTC instead of raising an exception #29606

Merged
merged 3 commits into from
Feb 19, 2023

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