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.

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

Move is_active user property to FAB auth manager #42042

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Sep 5, 2024

Clean up.

is_active is a property from FAB auth manager. We should not take care of it in Airflow but in FAB auth manager.


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member

potiuk commented Sep 6, 2024

I guess this means that Airlfow 3 will not work with earlier version of the FAB provider? Which I think is fine (I think it's quite a reasonable assumption that only "latest" version of FAB provider will work with Airflow 3), but it means that we have to bump version of the provider in the same PR and add fab_provider>= limit in preinstalled providers.

@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 6, 2024

I guess this means that Airlfow 3 will not work with earlier version of the FAB provider? Which I think is fine (I think it's quite a reasonable assumption that only "latest" version of FAB provider will work with Airflow 3), but it means that we have to bump version of the provider in the same PR and add fab_provider>= limit in preinstalled providers.

That's true, in that case an inactive user will be able to log in in Airflow 3 with earlier version of FAB. I am not sure if this inactive user feature is widely used but I guess to be on the safe side we can do what you propose

@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 6, 2024

I guess this means that Airlfow 3 will not work with earlier version of the FAB provider? Which I think is fine (I think it's quite a reasonable assumption that only "latest" version of FAB provider will work with Airflow 3), but it means that we have to bump version of the provider in the same PR and add fab_provider>= limit in preinstalled providers.

Done, please take a look to double check

@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 6, 2024

I guess this means that Airlfow 3 will not work with earlier version of the FAB provider? Which I think is fine (I think it's quite a reasonable assumption that only "latest" version of FAB provider will work with Airflow 3), but it means that we have to bump version of the provider in the same PR and add fab_provider>= limit in preinstalled providers.

Actually, does not it makes sense to do that before releasing Airflow 3? Several releases of FAB provider will happen before Airflow 3 is released, thus we might want to do it just before releasing Airflow 3 and set the latest FAB provider version?

@vincbeck vincbeck marked this pull request as draft September 6, 2024 21:26
@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 6, 2024

Converting the PR to draft to avoid accidental merging

@vincbeck
Copy link
Contributor Author

vincbeck commented Sep 9, 2024

I guess this means that Airlfow 3 will not work with earlier version of the FAB provider? Which I think is fine (I think it's quite a reasonable assumption that only "latest" version of FAB provider will work with Airflow 3), but it means that we have to bump version of the provider in the same PR and add fab_provider>= limit in preinstalled providers.

Actually, does not it makes sense to do that before releasing Airflow 3? Several releases of FAB provider will happen before Airflow 3 is released, thus we might want to do it just before releasing Airflow 3 and set the latest FAB provider version?

WDYT @potiuk ?

@vincbeck vincbeck marked this pull request as ready for review September 11, 2024 19:45
@vincbeck vincbeck merged commit a094f91 into apache:main Sep 12, 2024
56 checks passed
@vincbeck vincbeck deleted the vincbeck/is_active branch September 12, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants