You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "hussein-awala (via GitHub)" <gi...@apache.org> on 2023/02/18 20:38:58 UTC
[GitHub] [airflow] hussein-awala opened a new pull request, #29606: convert moment with timezone to UTC instead of raising an exception
hussein-awala opened a new pull request, #29606:
URL: https://github.com/apache/airflow/pull/29606
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #29606: convert moment with timezone to UTC instead of raising an exception
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #29606:
URL: https://github.com/apache/airflow/pull/29606#discussion_r1111206098
##########
airflow/triggers/temporal.py:
##########
@@ -41,10 +41,8 @@ def __init__(self, moment: datetime.datetime):
# Make sure it's in UTC
elif moment.tzinfo is None:
raise ValueError("You cannot pass naive datetimes")
- 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)
Review Comment:
"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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis merged pull request #29606: convert moment with timezone to UTC instead of raising an exception
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis merged PR #29606:
URL: https://github.com/apache/airflow/pull/29606
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a diff in pull request #29606: convert moment with timezone to UTC instead of raising an exception
Posted by "dimberman (via GitHub)" <gi...@apache.org>.
dimberman commented on code in PR #29606:
URL: https://github.com/apache/airflow/pull/29606#discussion_r1111314236
##########
airflow/triggers/temporal.py:
##########
@@ -41,10 +41,8 @@ def __init__(self, moment: datetime.datetime):
# Make sure it's in UTC
elif moment.tzinfo is None:
raise ValueError("You cannot pass naive datetimes")
- 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)
Review Comment:
@Taragolis looks like there are tests added. Do you think there should be more testing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29606: convert moment with timezone to UTC instead of raising an exception
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29606:
URL: https://github.com/apache/airflow/pull/29606#discussion_r1111270818
##########
airflow/triggers/temporal.py:
##########
@@ -41,10 +41,8 @@ def __init__(self, moment: datetime.datetime):
# Make sure it's in UTC
elif moment.tzinfo is None:
raise ValueError("You cannot pass naive datetimes")
- 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)
Review Comment:
I just updated the test `test_datetime_trigger_timing` to check if the logic remains valid with different timezones
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #29606: convert moment with timezone to UTC instead of raising an exception
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #29606:
URL: https://github.com/apache/airflow/pull/29606#discussion_r1111272142
##########
airflow/triggers/temporal.py:
##########
@@ -41,10 +41,8 @@ def __init__(self, moment: datetime.datetime):
# Make sure it's in UTC
elif moment.tzinfo is None:
raise ValueError("You cannot pass naive datetimes")
- 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)
Review Comment:
I guess we could check just in class constructor but this seems like even better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org