[airflow] Add misssing AIR302 attribute check #17115
Conversation
|
Lee-W
force-pushed
the
add-more-AIR303-condition
branch
from
c2f11da to
7aad00e
Compare
Lee-W
force-pushed
the
add-more-AIR303-condition
branch
from
7aad00e to
189cf32
Compare
dhruvmanila
added
rule
| AIR303.py:316:1: AIR303 Import path `airflow.api.auth.backend.basic_auth` is moved into `fab` provider in Airflow 3.0; | ||
| | | ||
| 315 | # apache-airflow-providers-cncf-kubernetes | ||
| 316 | ALL_NAMESPACES | ||
| 315 | # check whether attribute access | ||
| 316 | basic_auth.auth_current_user | ||
| | ^^^^^^^^^^ AIR303 | ||
| 317 | | ||
| 318 | # apache-airflow-providers-cncf-kubernetes | ||
| | | ||
| = help: Install `apache-airflow-provider-fab>=1.0.0` and import from `airflow.providers.fab.auth_manager.api.auth.backend.basic_auth` instead. | ||
|
|
||
| AIR303.py:316:12: AIR303 Import path `airflow.api.auth.backend.basic_auth` is moved into `fab` provider in Airflow 3.0; | ||
| | | ||
| 315 | # check whether attribute access | ||
| 316 | basic_auth.auth_current_user | ||
| | ^^^^^^^^^^^^^^^^^ AIR303 | ||
| 317 | | ||
| 318 | # apache-airflow-providers-cncf-kubernetes | ||
| | | ||
| = help: Install `apache-airflow-provider-fab>=1.0.0` and import from `airflow.providers.fab.auth_manager.api.auth.backend.basic_auth` instead. | ||
|
|
There was a problem hiding this comment.
This seems to be creating two diagnostics that are same for the two parts of the attribute node. Can you look into it?
There was a problem hiding this comment.
I think it's expected for "airflow", "api", "auth", "backend", "basic_auth", ... ...basic_auth or anything under basic_auth
There was a problem hiding this comment.
to reduce this kind of confusion. We're also going to remove Replacement::ImportPathMoved and expand them to individual Replacement::ProviderName instead.
But I think the logic in this PR is still required
There was a problem hiding this comment.
I'm not sure I follow here. There are two duplicate diagnostics for separate symbol:
basic_authauth_current_user
They both say:
Import path `airflow.api.auth.backend.basic_auth` is moved into `fab` provider in Airflow 3.0;
But, we should avoid the duplicate diagnostic and only highlight it once.
There was a problem hiding this comment.
I think basic_auth was check when it is passed into as a Name and auth_current_user is checked when it's an attribute under basic_auth (because of ["airflow", "api", "auth", "backend", "basic_auth", ..])
The issue occurs because we implemented the path check only instead of checking the exact name. This is antoher issue (which will be addressed when we replace all the path check with exact name check)
This PR intends to solve the following issue. (I'll update the test fixture)
from airflow.operators import python
python.PythonOperator()There was a problem hiding this comment.
I understand that part, but I think the bug related to duplicate diagnostic would still exists and we should address that. I'm not exactly sure how as I'd need to look at the code first.
There was a problem hiding this comment.
I just updated the test fixture to the issue we have.
Yep, we'll address that issue as well. but that would take sometime to replace Replacement::ImportPath as Replacement::Provider. @sunank200 and I already sync up and we're thinking of making them another PR. WDYT?
There was a problem hiding this comment.
It's also ok if we want it to be in this PR 🙂
There was a problem hiding this comment.
We want to do a release this week and I'm worried that this might not be fixed until then. I'd prefer that we do it in this PR itself.
Lee-W
force-pushed
the
add-more-AIR303-condition
branch
from
189cf32 to
5573369
Compare
Lee-W
force-pushed
the
add-more-AIR303-condition
branch
from
5573369 to
ebd06bb
Compare
Lee-W
changed the title
Summary
attribute check was missing in the previous implementation
e.g.
This PR adds this kind of check.
Test Plan
The test case has been added to the button of the existing test fixtures, confirmed to be correct and later reorgnaized