You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/08 16:43:08 UTC
[GitHub] [airflow] yergi opened a new issue #13570: `ImportChangesRule` raises false positives
yergi opened a new issue #13570:
URL: https://github.com/apache/airflow/issues/13570
**Apache Airflow version**: 1.10.14, apache-airflow-upgrade-check==1.1.0
**Kubernetes version (if you are using kubernetes)** (use `kubectl version`):
**Environment**:
- **Cloud provider or hardware configuration**: on-premise Linux server
- **OS** (e.g. from /etc/os-release): RHEL 7
- **Kernel** (e.g. `uname -a`):
- **Install tools**:
- **Others**:
**What happened**:
Running `airflow upgrade_check` gives a series of false positives for the import check rule. For example:
```
('Using `airflow.contrib.hooks.sftp_hook.SFTPHook` should be replaced by '
'`airflow.providers.sftp.hooks.sftp.SFTPHook`. Affected file: '
'/airflow/dags/std_wsljobs_dags/hccmodel_dag.py')
```
However, this import has actually already been updated in the indicated file, and when viewing the code using the "Code" button in the Airflow UI it shows the updated path.
**What you expected to happen**:
That this file would not be flagged by the ImportChangesRule.
**How to reproduce it**:
Run `airflow upgrade_check` with a DAG file containing one of the classes whose import path changed.
**Anything else we need to know**:
I believe the culprit is this section from the ImportChangesRule:
```
@staticmethod
def _check_file(file_path):
problems = []
providers = set()
with open(file_path, "r") as file:
content = file.read()
for change in ImportChangesRule.ALL_CHANGES:
if change.old_class in content:
problems.append(change.info(file_path))
if change.providers_package:
providers.add(change.providers_package)
return problems, providers
```
It is only checking the the `old_class` is present, not the `old_path`. The if statement should read: `if change.old_path in content:`. If I make this change in the source file, I receive zero problems back instead of 39.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
eladkal commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-811801612
@yergi were you able to verify ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
eladkal commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-788675640
@yergi can you please check if https://github.com/apache/airflow/pull/14493 solves your issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-757324306
Are you willing to create PR? I am happy to help with the review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] yergi commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
yergi commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-812003960
Yes this did it. Thank you!!
Get Outlook<https://aka.ms/qtex0l> for iOS
________________________________
From: eladkal ***@***.***>
Sent: Thursday, April 1, 2021 6:04:31 AM
To: apache/airflow ***@***.***>
Cc: James Colvin ***@***.***>; Mention ***@***.***>
Subject: Re: [apache/airflow] `ImportChangesRule` raises false positives (#13570)
@yergi<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fyergi&data=04%7C01%7C%7C5179bc931f7144f02fd408d8f4f58b9f%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637528682744009278%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=hMtTPDI7UuIOZLY3c1gekiqWA5Xy211Igc%2B%2FhM3AjyE%3D&reserved=0> were you able to verify ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fairflow%2Fissues%2F13570%23issuecomment-811801612&data=04%7C01%7C%7C5179bc931f7144f02fd408d8f4f58b9f%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637528682744019264%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=S1dgvNuRhzh6sv9DLr0xSQnDGBKiGEq290Rbkkv9kuY%3D&reserved=0>, or unsubscribe<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABZIHL7TVZ7LZNURQQ6PTETTGRAK7ANCNFSM4V2VZH7A&data=04%7C01%7C%7C5179bc931f7144f02fd408d8f4f58b9f%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637528682744019264%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BtvjudxtQ4A1P82G1KIwAHH7lrEblGu26Pbjw4njiM8%3D&reserved=0>.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-756861829
Thanks for opening your first issue here! Be sure to follow the issue template!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] yergi commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
yergi commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-758229903
Yes, I just created this PR. It's my first time ever contributing to an open-source project, so please be gentle. 😄
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal closed issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
eladkal closed issue #13570:
URL: https://github.com/apache/airflow/issues/13570
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zappallot commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
zappallot commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-781445228
I had the same problem with too many false positives and applied change proposed.
Problem is, it now does not find import statements like this:
`from airflow.contrib.operators.kubernetes_pod_operator import KubernetesPodOperator`
because `change.old_path` would expect a string like `airflow.contrib.operators.kubernetes_pod_operator.KubernetesPodOperator`.
Changing `change.old_path` to `".".join(change.old_path.split(".")[0:-1])` worked for me, as it cuts away the classname from the path. But I cannot say if this is good enough for the checker, but just `changes.old_path` would find not enough deprecated imports.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-756861829
Thanks for opening your first issue here! Be sure to follow the issue template!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-757324306
Are you willing to create PR? I am happy to help with the review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zappallot edited a comment on issue #13570: `ImportChangesRule` raises false positives
Posted by GitBox <gi...@apache.org>.
zappallot edited a comment on issue #13570:
URL: https://github.com/apache/airflow/issues/13570#issuecomment-781445228
I had the same problem with too many false positives and applied the change proposed.
Problem is, it now does not find import statements like this:
`from airflow.contrib.operators.kubernetes_pod_operator import KubernetesPodOperator`
because `change.old_path` would expect a string like `airflow.contrib.operators.kubernetes_pod_operator.KubernetesPodOperator`.
Changing `change.old_path` to `".".join(change.old_path.split(".")[0:-1])` worked for me, as it cuts away the classname from the path. But I cannot say if this is good enough for the checker, but just `changes.old_path` would find not enough deprecated imports.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org