Skip to content

Conversation

@jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Dec 27, 2025

Another small (in this case rather medium complex) increment to remove global statements for PR #58116

This removes global statements from plugins_manager.py where the a lot of state was hold in variables used with global and then a lot of code way relying to access these globals. Smelled a like a public interface and needed a bit of refactoring in other parts of the core to use proper access methods. Using new access methods with get_...() for all access with functools.cache() to prevent usage of global variables. All non external used methods marked as private to ensure it is not implicitly getting to be a public interface.

global is evil.

@potiuk potiuk changed the title Remove global from plaungs_manager Remove global from plugins_manager Dec 27, 2025
@potiuk
Copy link
Member

potiuk commented Dec 27, 2025

Why the .js changes?

@jscheffl jscheffl changed the title Remove global from plugins_manager [WIP] Remove global from plugins_manager Dec 27, 2025
@jscheffl
Copy link
Contributor Author

Why the .js changes?

Somehow the Prek plugin for JS compiler kicked-in after the small PY changes in FAB... not sure why. Seems some signature change...

@potiuk
Copy link
Member

potiuk commented Dec 27, 2025

Why the .js changes?

Somehow the Prek plugin for JS compiler kicked-in after the small PY changes in FAB... not sure why. Seems some signature change...

This one changed:

providers/fab/src/airflow/providers/fab/www/extensions/__init__.py

And it matches "www" of the provider:

"www": PROVIDERS_ROOT / "fab" / "src" / "airflow" / "providers" / "fab" / "www",

We should probably make it a bit more selective when we switch to hatch for both edge3 and fab - but it's okish as it is now.

@jscheffl jscheffl force-pushed the bugfix/remove-global-from-plugins-manager branch from c3227c5 to a1b9d32 Compare December 27, 2025 22:34
@jscheffl jscheffl added full tests needed We need to run full set of tests for this PR to merge all versions If set, the CI build will be forced to use all versions of Python/K8S/DBs labels Dec 28, 2025
@jscheffl jscheffl force-pushed the bugfix/remove-global-from-plugins-manager branch from 491484a to 4f7dea5 Compare December 28, 2025 17:35
@jscheffl jscheffl changed the title [WIP] Remove global from plugins_manager Remove global from plugins_manager Dec 28, 2025
@jscheffl jscheffl marked this pull request as ready for review December 28, 2025 17:37
@jscheffl
Copy link
Contributor Author

We should probably make it a bit more selective when we switch to hatch for both edge3 and fab - but it's okish as it is now.

Changes the config in this PR now as the JS changes generated false alarms and this bothered me. Ready for review now!

@jscheffl jscheffl requested a review from potiuk December 28, 2025 18:55
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Good call!

@potiuk potiuk merged commit 24d95ec into apache:main Dec 28, 2025
452 of 461 checks passed
Subham-KRLX pushed a commit to Subham-KRLX/airflow that referenced this pull request Jan 2, 2026
* Remove global from plaungs_manager

* Further rework and cleanup

* Prevent re-build of Fab assets, change exclusions

* Fixes

* Fixes

* Fixes

* Fixes
stegololz pushed a commit to stegololz/airflow that referenced this pull request Jan 9, 2026
* Remove global from plaungs_manager

* Further rework and cleanup

* Prevent re-build of Fab assets, change exclusions

* Fixes

* Fixes

* Fixes

* Fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

all versions If set, the CI build will be forced to use all versions of Python/K8S/DBs area:API Airflow's REST/HTTP API area:lineage area:plugins area:providers area:serialization full tests needed We need to run full set of tests for this PR to merge provider:fab

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants