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

Handle AUTH_ROLE_PUBLIC in FAB auth manager #42280

Merged
merged 10 commits into from
Sep 20, 2024

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Sep 17, 2024

AUTH_ROLE_PUBLIC is an option of FAB auth manager, it should not be handled in Airflow.


^ 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.

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

We discussed in Airflow 3 Devcall that Provider changes and Core adjustments should not be mixed. Can you please separate the changes?

As it seems dependencies between provider and core are adjusted - are you sure the provider will stay compatible with Airflow 2.9?

@vincbeck
Copy link
Contributor Author

We discussed in Airflow 3 Devcall that Provider changes and Core adjustments should not be mixed. Can you please separate the changes?

As it seems dependencies between provider and core are adjusted - are you sure the provider will stay compatible with Airflow 2.9?

Yes, similar to #42042, both changes (providers and core) are interrelated and it is impossible to split them. I am basically moving some features from core to providers so the provider will be compatible with Airflow 2.9. The only incompatibility is Airflow 3 with current or older version of FAB (because the feature is neither in Airflow and providers) but we decided (see comment) that Airflow 3 will require last version of Fab

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looking nice. So that means I can safely remove the TODO comment in my other PR.

As I understand the backend is now checking automatically the AUTH_ROLE_PUBLIC if the user is None that's cleaner 👍

@vincbeck
Copy link
Contributor Author

I cannot reproduce and understand why the compat tests are failing. I need to debug directly in the PR. Setting the PR in draft until I figure it out

@vincbeck vincbeck marked this pull request as draft September 19, 2024 15:54
@vincbeck vincbeck force-pushed the vincbeck/auth_role_public branch 2 times, most recently from ea83a4d to f556800 Compare September 19, 2024 16:44
@vincbeck vincbeck force-pushed the vincbeck/auth_role_public branch 4 times, most recently from 0f58a5a to 20a1656 Compare September 19, 2024 19:21
@jscheffl
Copy link
Contributor

I cannot reproduce and understand why the compat tests are failing. I need to debug directly in the PR. Setting the PR in draft until I figure it out

Have you tried to run the compat tests local? Don't know how powerful your laptop is but for me this is a faster round trip.

breeze testing tests --python 3.8 --run-in-parallel --parallel-test-types "Providers[whatever_you_want]" --use-packages-from-dist --package-format wheel --use-airflow-version "2.8.4" --airflow-constraints-reference constraints-2.8.4 --install-airflow-with-constraints --providers-skip-constraints --skip-providers "cloudant fab"

@vincbeck
Copy link
Contributor Author

I cannot reproduce and understand why the compat tests are failing. I need to debug directly in the PR. Setting the PR in draft until I figure it out

Have you tried to run the compat tests local? Don't know how powerful your laptop is but for me this is a faster round trip.

breeze testing tests --python 3.8 --run-in-parallel --parallel-test-types "Providers[whatever_you_want]" --use-packages-from-dist --package-format wheel --use-airflow-version "2.8.4" --airflow-constraints-reference constraints-2.8.4 --install-airflow-with-constraints --providers-skip-constraints --skip-providers "cloudant fab"

Yep I did, it complains because the amazon provider package is not installed. I could not figure it out why. But I actually found out the reason why the tests are failing by debugging by pushing things and waiting the CI for feedback :D

@vincbeck vincbeck marked this pull request as ready for review September 19, 2024 20:43
@vincbeck
Copy link
Contributor Author

CI is (finally) green!

@vincbeck vincbeck merged commit 8741e9c into apache:main Sep 20, 2024
56 checks passed
@vincbeck vincbeck deleted the vincbeck/auth_role_public branch September 20, 2024 20:52
joaopamaral pushed a commit to joaopamaral/airflow that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:providers area:webserver Webserver related Issues provider:fab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants