-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Clarify pendulum use in timezone cases #21646
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits - lgtm
Should all of the example DAGs get updated as well? This feels like a similar preemptive move we did to prevent user headaches by adding |
I think we are using "timezone" more frequently than "time zone". Just a little mixing of the two in places. |
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
That's a good idea. The examples are not "wrong" per se but indeed converting them to use pendulum might prevent users from adding non-pendulum timezones and lead them in the right direction. |
a0c486f
to
815ebdb
Compare
I rebased/Updated all example dags (non-providers for now) and rst examples to explicitly mention pendulum.datetime + timezone and datetime.timedelta. This I think is the best way to educate our users on using timezone. |
815ebdb
to
0e0d4ce
Compare
It is important to use Pendulum in case timezone is used - because there are a number of limitations coming from using stdlib timezone implementation. However our documentation was not very clear about it, especially some examples shown using standard datetime in DAGs which could mislead our users to continue using datetime if they use timezone. This PR clarifies and stresses the use of pendulum is necessary when timezone is used. Also it points to the documentation in case serialization throws error about not using Pendulum so that the users can learn about the reasoning. This is the first part of the change - the follow up will be changing all provider examples to also use timezone and pendulum explicitely. See also apache#20070
0e0d4ce
to
74cbc22
Compare
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Ah... Hawkeye :) |
It is important to use Pendulum in case timezone is used - because there are a number of limitations coming from using stdlib timezone implementation. However our documentation was not very clear about it, especially some examples shown using standard datetime in DAGs which could mislead our users to continue using datetime if they use timezone. This PR clarifies and stresses the use of pendulum is necessary when timezone is used. Also it points to the documentation in case serialization throws error about not using Pendulum so that the users can learn about the reasoning. This is the first part of the change - the follow up will be changing all provider examples to also use timezone and pendulum explicitly. See also #20070 (cherry picked from commit f011da2)
This aligns our DAGs with the Airflow example DAGs. See apache/airflow#21646.
* Use latest actions * Use pendulum instead of datetime for a DAG's start_date This aligns our DAGs with the Airflow example DAGs. See apache/airflow#21646. * Update GitHub Actions with dependabot * Remove outdated file, directory doesn't exist any more * Add include directory to pylint * Update Airflow to 2.2.5
It is important to use Pendulum in case timezone is used - because
there are a number of limitations coming from using stdlib
timezone implementation.
However our documentation was not very clear about it, especially
some examples shown using standard datetime in DAGs which could
mislead our users to continue using datetime if they use timezone.
This PR clarifies and stresses the use of pendulum is necessary
when timezone is used. Also it points to the documentation
in case serialization throws error about not using Pendulum
so that the users can learn about the reasoning.
See also #20070
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.