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

Make the method BaseAuthManager.is_authorized_custom_view abstract #37915

Merged
merged 14 commits into from
Mar 15, 2024

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Mar 5, 2024

While testing auth managers I realized that is_authorized_custom_view is also called when custom pages are defined in plugins. Therefore, we need to make the method BaseAuthManager.is_authorized_custom_view abstract so that auth managers can also work in environments with custom pages defined in plugins.

This is technically a breaking change but:

  • Auth managers have been introduced recently, I am very confident nobody used the interface yet
  • Documentation related to auth managers have not been released yet (will be in 2.9), hence the likelihood of having user using the interface is even less
  • This is a bug fix

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

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:providers area:webserver Webserver related Issues provider:amazon AWS/Amazon - related issues provider:fab labels Mar 5, 2024
@vincbeck vincbeck added this to the Airflow 2.9.0 milestone Mar 5, 2024
@vincbeck vincbeck requested a review from potiuk as a code owner March 7, 2024 15:03
Copy link
Contributor

@o-nikolas o-nikolas left a comment

Choose a reason for hiding this comment

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

I'm also a bit confused by the menu related changes.

Also is it possible to get unit test coverage of the plugin path? Since it was missed before

@vincbeck
Copy link
Contributor Author

vincbeck commented Mar 8, 2024

Hmmm, I can understand the MENU change looks unrelated. I can possibly split the PR in 2 if that helps. I can also a unit test to cover the plugin path

@vincbeck
Copy link
Contributor Author

vincbeck commented Mar 8, 2024

Actually, let me explain why I combined the two. The idea initially was to call is_authorized_custom_viewin this case: https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py#L512. This case is when there is a custom menu item in the menu added by a plugin. This was my initial idea. But since recently in #37881 we added the action MENU, the idea was to use it as well. I can definitely do it in 2 PRs but that would mean implementing something with unit tests associated, and deleting this exact same code I just implemented in a follow-up PRs to take into consideration the action MENU.

Though, I am still happy to make this effort if you think that would make the review easier

@vincbeck
Copy link
Contributor Author

vincbeck commented Mar 8, 2024

I also added a unit test that loads the index page. The difference with the test being implemented in #37947, is this one is a unit test. No real API call is made against AWS. This test will fail as long as #37997 is not merged.

@vincbeck
Copy link
Contributor Author

@o-nikolas @potiuk any concerns on why I also included the "MENU" change in this PR? Happy to change something if needed

@potiuk
Copy link
Member

potiuk commented Mar 13, 2024

LGTM

@vincbeck vincbeck merged commit 2e35854 into apache:main Mar 15, 2024
56 checks passed
@vincbeck vincbeck deleted the vincbeck/custom_view branch March 15, 2024 13:57
@ephraimbuddy ephraimbuddy added type:improvement Changelog: Improvements type:bug-fix Changelog: Bug Fixes and removed type:improvement Changelog: Improvements labels Mar 18, 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:amazon AWS/Amazon - related issues provider:fab type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants