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.

Sign up for GitHub

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

Jump to bottom

Remove FAB db manager as an external_db_managers default #48070

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Remove FAB db manager as an external_db_managers default #48070

wants to merge 15 commits into from
+207 −444

Conversation

jedcunningham
Copy link
Member

Now that the FAB auth manager isn't the default auth manager, it doesn't makes sense to have the FAB db manager as a default in external_db_managers.

vincbeck
vincbeck reviewed Mar 21, 2025
View reviewed changes
Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

I am wondering if the config is the right option. Is it something that auth managers should set? I do not see a scenario where a user use FAB auth manager but not FABDBManager and vice versa. Should not we have a method in the auth manager interface like get_db_manager that FAB auth manager overrides?

@jedcunningham
Copy link
Member Author

We could refactor it to expose it from auth managers, that does make sense. But this should be done to simply the auth manager use case, not that auth managers are the only use case for db managers :)

Would you oppose doing that in a follow up?

@jedcunningham
Copy link
Member Author

@ephraimbuddy looks like we have some core/fab migration mixing going on. Is that something you could take a stab at?

@vincbeck
Copy link
Contributor

We could refactor it to expose it from auth managers, that does make sense. But this should be done to simply the auth manager use case, not that auth managers are the only use case for db managers :)

Would you oppose doing that in a follow up?

Yep I can do it as a follow-up

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy looks like we have some core/fab migration mixing going on. Is that something you could take a stab at?

Sure. Looking

@ephraimbuddy ephraimbuddy force-pushed the remove_fab_db_manager_default branch from ca056a7 to e99a2d7 Compare March 24, 2025 14:20
@vincbeck vincbeck mentioned this pull request Mar 24, 2025
Merged
@vincbeck
Copy link
Contributor

We could refactor it to expose it from auth managers, that does make sense. But this should be done to simply the auth manager use case, not that auth managers are the only use case for db managers :)

Would you oppose doing that in a follow up?

#48196

@ephraimbuddy ephraimbuddy force-pushed the remove_fab_db_manager_default branch 2 times, most recently from 7d8ac3a to f9068ae Compare March 25, 2025 12:12
jedcunningham
jedcunningham commented Mar 25, 2025
View reviewed changes
@ephraimbuddy ephraimbuddy force-pushed the remove_fab_db_manager_default branch 2 times, most recently from bf25391 to 65b52d0 Compare March 25, 2025 21:06
@ephraimbuddy ephraimbuddy force-pushed the remove_fab_db_manager_default branch 2 times, most recently from 96c43c7 to 0c95b2a Compare March 26, 2025 15:16
@ephraimbuddy ephraimbuddy requested a review from vincbeck March 27, 2025 08:08
vincbeck
vincbeck reviewed Mar 27, 2025
View reviewed changes
jedcunningham and others added 15 commits March 28, 2025 21:58
@jedcunningham @ephraimbuddy
Remove FAB db manager as an external_db_managers default
2527344 
Now that the FAB auth manager isn't the default auth manager, it doesn't
makes sense to have the FAB db manager as a default in
`external_db_managers`.
@ephraimbuddy
Update migration to work when FAB is not among configured DB managers
f735d7d
@ephraimbuddy
Sync perms only when using FABAuthManager
6f6876d
@ephraimbuddy
Run tests with or without the fabauthmanager
9708688
@ephraimbuddy
Fix tests
72e327d
@jedcunningham @ephraimbuddy
Auth managers can now provide their own db manager!
91decce
@ephraimbuddy @jedcunningham
Update airflow-core/src/airflow/dag_processing/collection.py
ce07ba7 
Co-authored-by: Jed Cunningham <[email protected]>
@ephraimbuddy
Make 'ab_user' table a hard requirement when user is downgrading belo…
a2e4fa9 
…w 3.0.0
@ephraimbuddy
Apply suggestions from code review
35ce205
@ephraimbuddy
Fix whitespace in migration test
dbcb4ea
@ephraimbuddy
use env var for migration test
ae641cd
@ephraimbuddy
properly handle downgrade check
c478675
@ephraimbuddy
fixup! properly handle downgrade check
dc523e1
@ephraimbuddy
Update .pre-commit-config.yaml
2515b1c
@ephraimbuddy
apply suggestions from code review
fccfc7e
@ephraimbuddy ephraimbuddy force-pushed the remove_fab_db_manager_default branch from 496a290 to fccfc7e Compare March 28, 2025 21:02
@ephraimbuddy ephraimbuddy requested a review from vincbeck March 28, 2025 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ephraimbuddy ephraimbuddy ephraimbuddy left review comments

@potiuk potiuk Awaiting requested review from potiuk potiuk is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@dstandish dstandish Awaiting requested review from dstandish

@hussein-awala hussein-awala Awaiting requested review from hussein-awala

@gopidesupavan gopidesupavan Awaiting requested review from gopidesupavan gopidesupavan is a code owner

@vincbeck vincbeck Awaiting requested review from vincbeck

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area:dev-tools area:helm-chart Airflow Helm Chart
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jedcunningham @vincbeck @ephraimbuddy