Skip to content

Conversation

@vincbeck
Copy link
Contributor

As reported in #47662, there is a bug in the method get_menu_items of 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.

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.

Checking on my phone so I might be missing something as it’s hard to read, but I am not sure how this will fix the issue on the fast api side. This will yield an exception and we will not be able to get the items are we ?

@vincbeck vincbeck force-pushed the vincbeck/fix_get_menu_items branch from e23ea96 to f6c7ec3 Compare March 12, 2025 18:26
@vincbeck
Copy link
Contributor Author

Checking on my phone so I might be missing something as it’s hard to read, but I am not sure how this will fix the issue on the fast api side. This will yield an exception and we will not be able to get the items are we ?

You're right. I also realized that the URLs do not change between the different implementation (DB, LDAP, Kerberos ...). So I simplified it and hardcoded the URLs. Simple is better

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.

Nice thanks

@Lee-W Lee-W merged commit 1c9c5c6 into apache:main Mar 13, 2025
60 checks passed
@vincbeck vincbeck deleted the vincbeck/fix_get_menu_items branch April 2, 2025 13:20
nailo2c pushed a commit to nailo2c/airflow that referenced this pull request Apr 4, 2025
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.

5 participants