Do not use core Airflow Flask related resources in FAB provider (tests of www)
#45472
Conversation
vincbeck
force-pushed
the
vincbeck/www-tests
branch
4 times, most recently
from
2913b83 to
8852b69
Compare
providers/src/airflow/providers/fab/www/extensions/init_appbuilder.py
Outdated
Show resolved
Hide resolved
| app.register_error_handler(ProblemException, common_error_handler) | ||
|
|
||
|
|
||
| def init_api_connexion(app: Flask) -> None: |
There was a problem hiding this comment.
We are probably at a point we can rip out the old api, to be honest - I think aip-84 is far enough along.
If we want to keep it for now, that's fine, however we might want to leave a breadcrumb that this needs to go before AF3.
There was a problem hiding this comment.
I agree. Though, removing it make some tests fail. Some tests in FAB provider are using Rest API endpoints (e.g. /api/v1/pools in providers/tests/fab/auth_manager/api_endpoints/test_auth.py). So we need to update these tests as well. I prefer doing it in a separate PR because I think this one is big enough already.
providers/src/airflow/providers/fab/www/extensions/init_views.py
Outdated
Show resolved
Hide resolved
vincbeck
force-pushed
the
vincbeck/www-tests
branch
3 times, most recently
from
f5b453c to
bfc0cad
Compare
There was a problem hiding this comment.
Copilot reviewed 14 out of 29 changed files in this pull request and generated 2 comments.
Files not reviewed (15)
- providers/src/airflow/providers/fab/www/templates/airflow/no_roles_permissions.html: Language not supported
- providers/tests/fab/auth_manager/api_endpoints/test_dag_run_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_dag_source_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_asset_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_variable_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_import_error_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_event_log_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_dag_warning_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_dag_endpoint.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_xcom_endpoint.py: Evaluated as low risk
- providers/src/airflow/providers/fab/www/app.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api/auth/backend/test_basic_auth.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api/auth/backend/test_session.py: Evaluated as low risk
- providers/src/airflow/providers/fab/auth_manager/security_manager/override.py: Evaluated as low risk
- providers/tests/fab/auth_manager/api_endpoints/test_auth.py: Evaluated as low risk
Comments suppressed due to low confidence (3)
providers/src/airflow/providers/fab/www/extensions/init_views.py:135
- The method_not_allowed function should be imported at the top of the file for clarity and to avoid potential issues with circular imports.
from airflow.providers.fab.www.views import method_not_allowed
providers/src/airflow/providers/fab/www/extensions/init_views.py:111
- [nitpick] Encapsulate the base_paths list within a class to avoid potential side effects and make the code more maintainable.
base_paths: list[str] = []
providers/src/airflow/providers/fab/www/views.py:53
- [nitpick] The error message is too generic. It should be updated to 'Method Not Allowed: The requested method is not allowed for the requested URL.'
error_message="Received an invalid request."
| if allow_origins == "*": | ||
| response.headers["Access-Control-Allow-Origin"] = "*" | ||
| elif allow_origins: | ||
| allowed_origins = allow_origins.split(" ") |
There was a problem hiding this comment.
The allow_origins variable should be split by commas instead of spaces to handle multiple origins more accurately.
| allowed_origins = allow_origins.split(" ") | |
| allowed_origins = allow_origins.split(",") |
| # here on the application level | ||
| return common_error_handler(ex) | ||
| else: | ||
| from airflow.providers.fab.www.views import not_found |
There was a problem hiding this comment.
The not_found function should be imported at the top of the file for clarity and to avoid potential issues with circular imports.
| from airflow.providers.fab.www.views import not_found | |
| return not_found(ex) |
vincbeck
force-pushed
the
vincbeck/www-tests
branch
4 times, most recently
from
338485a to
4faa6cf
Compare
vincbeck
force-pushed
the
vincbeck/www-tests
branch
from
4faa6cf to
4ac18c2
Compare
|
Closing this PR since I actually do not think this is needed. All these tests will be deleted when the Airflow 2 UI is deleted |
2 participants
Follow-up of #45441.
In #45441 I forgot to apply the changes in tests as well. This PR solves that.
^ 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.rstor{issue_number}.significant.rst, in newsfragments.