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

Add support for ZoneInfo and generic UTC #34683

Merged
merged 11 commits into from
Oct 6, 2023

Conversation

bolkedebruin
Copy link
Contributor

Certain providers rely on other datetime implementations than pendulum and fail to serialize.

See also: #34673

@Taragolis @ferruzzi


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Certain providers rely on other datetime implementations
and fail to serialize.
setup.py Outdated Show resolved Hide resolved
airflow/serialization/serializers/timezone.py Show resolved Hide resolved
@ferruzzi
Copy link
Contributor

I'd like time to pull these changes and see if it fixes the issue that the system test caught rather than just looking at the code and guessing, but I might not get to that until Tuesday. If anyone gets time to do that, I'd greatly appreciate the help and confirmation.

Test request: Pull these changes down and successfully execute a DAG which passes the timestamp from boto3.client("dynamodb").describe_continuous_backups(TableName=table_name)["ContinuousBackupsDescription"]["PointInTimeRecoveryDescription"]["EarliestRestorableDateTime"] into XCOM and then retrieving it.

Comment on lines +72 to +74
i = DateTime(2022, 7, 10, tzinfo=tzutc())
s = serialize(i)
d = deserialize(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ferruzzi, IMO your question about encode/decode datetimes from boto3 covered by this test case

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are likely right, ad the code does look good "on paper", I was just hoping to make time to actually try it in a live environment to verify. I won't block the merge if you are confident. 👍

@bolkedebruin
Copy link
Contributor Author

I'd like time to pull these changes and see if it fixes the issue that the system test caught rather than just looking at the code and guessing, but I might not get to that until Tuesday. If anyone gets time to do that, I'd greatly appreciate the help and confirmation.

Test request: Pull these changes down and successfully execute a DAG which passes the timestamp from boto3.client("dynamodb").describe_continuous_backups(TableName=table_name)["ContinuousBackupsDescription"]["PointInTimeRecoveryDescription"]["EarliestRestorableDateTime"] into XCOM and then retrieving it.

Tuesday is fine, we're not in a hurry. Rather have you confirm it, although the tests should now cover it too.

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

I thought I could checkout this PR and run it, but if that's possible, I can't seem to figure it out. The code does look good, I was just hoping to verify it in action. Going to approve it as-is.

setup.cfg Outdated Show resolved Hide resolved
@bolkedebruin bolkedebruin merged commit 7707f4a into apache:main Oct 6, 2023
43 of 65 checks passed
@bolkedebruin
Copy link
Contributor Author

Oops, seeing that tests are failing due to import errors. Looking into it.

@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.2 milestone Oct 6, 2023
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Oct 6, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 7, 2023
* Add support for ZoneInfo and generic UTC

Certain providers rely on other datetime implementations
and fail to serialize.

(cherry picked from commit 7707f4a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants