Skip to content

Improve handling of backwards compat for airflow.io#36199

Merged
bolkedebruin merged 5 commits into
apache:mainfrom
bolkedebruin:improve_backwards_compat
Dec 13, 2023
Merged

Improve handling of backwards compat for airflow.io#36199
bolkedebruin merged 5 commits into
apache:mainfrom
bolkedebruin:improve_backwards_compat

Conversation

@bolkedebruin

@bolkedebruin bolkedebruin commented Dec 13, 2023

Copy link
Copy Markdown
Contributor

Older providers do not have a get_fs method that takes storage_options as arguments. If we encounter such provider and storage_options are passed we should error out instead if silently ignoring.

@potiuk

@ephraimbuddy if we do a rc3 for 2.8 this would be great to have in.

Closes: #36187


^ 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.

Older providers do not have a get_fs method that takes
storage_options as arguments. If we encounter such provider
and storage_options are passed we should error out instead
if silently ignoring.
@bolkedebruin bolkedebruin requested a review from potiuk December 13, 2023 09:27

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Much more complete. @ephraimbuddy - if we have a chance to get it into RC2 still (or RC3 is on the horizon) this one is very good candidate.

Comment thread tests/io/test_path.py Outdated
get_fs("file", storage_options={"foo": "bar"})

# Reset the cache to avoid side effects
_register_filesystems.cache_clear()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice. Recalls the recent cache clearing issue :)

@potiuk potiuk Dec 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But I think It would be better to use try/finally or - better - pytest fixture WITH try/finally. This way the cache will be cleared even if the test fails @bolkedebruin

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see #35746

Comment thread airflow/io/__init__.py Outdated
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@ephraimbuddy

Copy link
Copy Markdown
Contributor

Much more complete. @ephraimbuddy - if we have a chance to get it into RC2 still (or RC3 is on the horizon) this one is very good candidate.

I have already released RC2. Maybe RC3 but tests are failing

@ephraimbuddy

Copy link
Copy Markdown
Contributor

Anyone working on this or should we leave it for 2.8.1?

@bolkedebruin

Copy link
Copy Markdown
Contributor Author

Im working on it but I'm also not feeling so well. So I'm a bit slow.

@bolkedebruin

Copy link
Copy Markdown
Contributor Author

Give me an hour or so

@ephraimbuddy

Copy link
Copy Markdown
Contributor

Im working on it but I'm also not feeling so well. So I'm a bit slow.

Sorry about that. Take it easy

@bolkedebruin bolkedebruin merged commit 6c94ddf into apache:main Dec 13, 2023
ephraimbuddy pushed a commit that referenced this pull request Dec 13, 2023
Older providers do not have a get_fs method that takes
storage_options as arguments. If we encounter such provider
and storage_options are passed we should error out instead
if silently ignoring.

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
(cherry picked from commit 6c94ddf)
@bolkedebruin bolkedebruin deleted the improve_backwards_compat branch December 13, 2023 18:04
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Jan 10, 2024
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit tests to retrieve fsspec from providers including backwards compatibility

5 participants