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

Revert "check whether AUTH_ROLE_PUBLIC is set in check_authentication… #39009

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Apr 14, 2024

… (#38924)"

This reverts commit 7b60825.


^ 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 potiuk requested a review from jhtimmins as a code owner April 14, 2024 14:50
@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Apr 14, 2024
@potiuk
Copy link
Member Author

potiuk commented Apr 14, 2024

Hey @Lee-W - this PR has a side effect on failing a number of tests in

tests/providers/fab/auth_manager/api_endpoints/test_role_and_permission_endpoint.py

By a quick look, change from 401 -> 403 impacts all the cases where unauthanticated user is used, which I think is not an intended behaviour (the original isssue mentions only AUTH_ROLE_PUBLIC case). So rather than fixing all 401 in 403 - I think it's better to revert it now and fix it "properly" (which also means that full tests needed should be set as label for the new PR). I will also add FAB provider tests to be triggered in selective checks when "api" tests are run - because this is the reason why it did not fail in your PR originally.

@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 14, 2024
@potiuk potiuk closed this Apr 14, 2024
@potiuk potiuk reopened this Apr 14, 2024
@potiuk potiuk merged commit 7fc2169 into apache:main Apr 14, 2024
91 of 101 checks passed
@potiuk potiuk deleted the revert-38924-temporarily branch April 14, 2024 15:24
@potiuk
Copy link
Member Author

potiuk commented Apr 14, 2024

The PR to run FAB tests together with API changes is merged now @Lee-W -> so you should be able to redo the PR and see the tess failing in your PR. The "too selective" case should be now nicely covered.

Lee-W added a commit to astronomer/airflow that referenced this pull request Apr 15, 2024
@Lee-W
Copy link
Member

Lee-W commented Apr 15, 2024

Sure. I just addressed the issue, created a new PR #39012, and added the required label. Thanks for reminding me!

@potiuk
Copy link
Member Author

potiuk commented Apr 15, 2024

Sure. I just addressed the issue, created a new PR #39012, and added the required label. Thanks for reminding me!

Sorry for the troubles with selective checs :( . But should be fixed from now on.

@Lee-W
Copy link
Member

Lee-W commented Apr 15, 2024

Sure. I just addressed the issue, created a new PR #39012, and added the required label. Thanks for reminding me!

Sorry for the troubles with selective checs :( . But should be fixed from now on.

Not at all. Thanks for the fix 🙂

utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
@utkarsharma2 utkarsharma2 added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jun 4, 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 changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants