-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Remove core references in secrets backend logic in sdk #59597
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
Remove core references in secrets backend logic in sdk #59597
Conversation
|
Should not secret search path be passed as an initialization parameter from outside ? I think we should note have "ANY" like ANY references to core packages from shared libraries. The only references it should have is via releative imports to other shared libraries. Not even "string" form of those. Actually we might even want to extend my #58825 to also check for those. In this case - an obvious solution is to have some kind of mandatory intialization method that should be called by the user and pass those paths. Then "airflow-core" could pass it's own paths, and 'task-sdk` could pass it's own - no coupling, no core nor task-sdk code in the shared library. I think we might even come up with some convention on how all "shared" libraries should be initialized by it's users - and have some common |
|
@potiuk pretty good idea really. Let me take a stab at this one. |
shared/secrets_backend/src/airflow_shared/secrets_backend/__init__.py
Outdated
Show resolved
Hide resolved
|
Is this good to merge? |
|
Some comments from @potiuk that I have to look at, let me do it now. |
potiuk
left a comment
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.
Nit - would be nice to keep it in sync with prek hook
|
Thanks, I added a prek hook which does this for mismatch: (apache-airflow) ➜ airflow git:(remove-dependency-on-constant) ✗ ./scripts/ci/prek/check_secrets_search_path_sync.py
ERROR: Secrets search paths are not synchronized!
The DEFAULT_SECRETS_SEARCH_PATH in airflow-core and _SERVER_DEFAULT_SECRETS_SEARCH_PATH in task-sdk must match.
Please update either:
- /Users/amoghdesai/Documents/OSS/repos/airflow/airflow-core/src/airflow/secrets/base_secrets.py
- /Users/amoghdesai/Documents/OSS/repos/airflow/task-sdk/src/airflow/sdk/execution_time/secrets/__init__.py |
jason810496
left a comment
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.
Just ran the prek script manually, LGTM.
|
nice! |
We imported around:
DEFAULT_SECRETS_SEARCH_PATHfromairflow.secretsin sdk configuration to detect worker mode and perform some operations with that.We indeed need the constant for some detection logic, so instead of cross importing, we can just maintain a copy in the task sdk, since thats unlikely to change.
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.