-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Fix plugin macros not being exposed through airflow.macros #12788
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
39a4e88
Add a test that reproduces airflow#12785
a6eec0c
Fix plugin-provided macros not being exposed on airflow.macros
6ba9f40
Use plugin.name instead of splitting the module name
36af916
Add cleanup logic to test_registering_plugin_macros
087c74f
Remove access to airflow.macros for subprocesses
348ffba
Revert "Remove access to airflow.macros for subprocesses"
b771370
Integrate plugin-provided macros in subprocesses
cee7686
Implement a more strict version-check
cb64e96
Document macros limitation
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
integrate_macros_plugins()
has the side-effect that it modifies theairflow.macros
module. Do we need to add additional cleanup logic to reset the contents ofairflow.macros
through a finalizer here?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.
Will
importlib.reload
help?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.
Simply reloading the module using
importlib.reload()
will not be sufficient as the module's symbol table (dictionary) is retained.I think we'll have to delete the module from
sys.modules
and import it again in order to properly remove the entries that are being added by this test case.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.
I've implemented this in c2b5354 -- I'm curious to your thoughts.
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.
I love this change.😻 I'm just wondering if it's worth moving this code to the context manager mock_plugins_manager. This allows us to limit the side effects in other tests as well. What do you think?
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.
I considered doing this, but the
__doc__
comment on themock_plugin_manager
seems to suggest that the scope of that mock is limited to theairflow.plugins
module.While that mock does in fact clear out the
macros_modules
variable inairflow.plugins
, it does not actually attempt to reverse any (side) effects that are caused by invokingintegrate_macros_plugins()
. I did a bit of searching through the code base, and it doesn't really appear that there is any test coverage for this function beyond the test I've just added.Because I want to avoid scope creep for the
mock_plugin_manager
fixture, and this is the only test case that actually appears to have to deal with side effects cause by callingintegrate_macros_plugins()
, I'm a bit hesitant to make changes beyond what's being proposed here.