Skip to content

Make auth managers provide their own airflow CLI commands - #33481

Merged
o-nikolas merged 14 commits into
apache:mainfrom
aws-mwaa:vandonr/fab
Aug 23, 2023
Merged

Make auth managers provide their own airflow CLI commands#33481
o-nikolas merged 14 commits into
apache:mainfrom
aws-mwaa:vandonr/fab

Conversation

@vandonr-amz

@vandonr-amzvandonr-amz commented Aug 17, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

closes#32206
closes#32208
closes#32348

taking a lot of inspiration from #29055
Since auth managers are going to be configurable, we don't want their specific commands to be defined in airflow core.
Thus, this PR moves (almost) everything auth manager-related to the fab corner, and adds a mechanism so that auth managers can provide their own CLI commands.

On my computer, running this with the FAB auth manager makes the commands take 10 to 15 extra milliseconds, which seems reasonable.

vincbeck
vincbeck requested changes Aug 18, 2023
View reviewed changes
Comment threadairflow/auth/managers/fab/cli_commands/utils.py
Comment threadairflow/cli/commands/standalone_command.py
# with the reasoning that if you can read that directory, you can see
# the database credentials anyway.
from airflow.utils.cli_app_builder import get_application_builder
from airflow.auth.managers.fab.cli_commands.utils import get_application_builder

@vincbeckvincbeckAug 18, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not good. We do not want Airflow to have a dependency on FAB auth manager. People might use another Auth manager and not even have FAB auth manager installed (this is assuming FAB auth manager is moved to a separate provider)

@vandonr-amzvandonr-amzAug 21, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yes I know, we talked a bit about it on Thursday when we looked at the code together :)
I moved the "easy" commands in the PR, this one will need to move (at least partially) too, but the situation is a little bit more complex, I suggest dealing with it in a separate PR, where I'd introduce an init_airflow method in auth managers for instance.

@vincbeckvincbeckAug 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am okay with that but we definitely do not want to forget it. This is something we really dont want

Comment threadairflow/providers/celery/executors/celery_executor.py
@vincbeck

vincbeck commented Aug 18, 2023

Copy link
Copy Markdown
Contributor

Does it resolve #32348 as well?

@vincbeckvincbeck added the AIP-56 Extensible user management label Aug 18, 2023
@vandonr-amz

vandonr-amz commented Aug 21, 2023

Copy link
Copy Markdown
ContributorAuthor

Does it resolve #32348 as well?

ah yes indeed, I didn't see there was a separate ticket for this

o-nikolas
o-nikolas reviewed Aug 21, 2023
View reviewed changes
Comment threadairflow/providers/celery/executors/celery_executor.py
Comment threadairflow/cli/cli_parser.py Outdated
@vandonr-amz

vandonr-amz commented Aug 22, 2023

Copy link
Copy Markdown
ContributorAuthor

@vincbeck you left a "requesting changes" review, I think your concerns have been addressed ?

@vincbeck

vincbeck commented Aug 23, 2023

Copy link
Copy Markdown
Contributor

@vincbeck you left a "requesting changes" review, I think your concerns have been addressed ?

Approved

@vandonr-amzvandonr-amz mentioned this pull request Aug 23, 2023
Merged
@o-nikolas
o-nikolas merged commit b1a3b42 into apache:mainAug 23, 2023
@vandonr-amz
vandonr-amz deleted the vandonr/fab branch August 23, 2023 21:58
@eladkaleladkal mentioned this pull request Aug 25, 2023
Closed
63 tasks
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 2, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@o-nikolaso-nikolaso-nikolas approved these changes
@vincbeckvincbeckvincbeck approved these changes
@hussein-awalahussein-awalaAwaiting requested review from hussein-awala
@ryanahamiltonryanahamiltonAwaiting requested review from ryanahamilton
@ashbashbAwaiting requested review from ashb
@bbovenzibbovenziAwaiting requested review from bbovenzi
@pierrejeambrunpierrejeambrunAwaiting requested review from pierrejeambrun

Assignees

No one assigned

Labels

AIP-56Extensible user managementarea:CLIarea:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:celery

Projects

None yet

Milestone

No milestone

4 participants

@vandonr-amz@vincbeck@o-nikolas@ephraimbuddy