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 2020/12/29 21:12:31 UTC
[GitHub] [airflow] madison-ookla opened a new pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
madison-ookla opened a new pull request #13371:
URL: https://github.com/apache/airflow/pull/13371
Resolves #13349
This PR adds a more useful error message when processing files using the `AirflowMacroPluginRemovedRule` upgrade check. It also prompts users to add said files to the `.airflowignore` file (which resolved the issue in our case).
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-760545080
@potiuk Can this be merged?
----------------------------------------------------------------
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] madison-ookla commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550274197
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
+ file_path
Review comment:
I actually added this initially! But because this is against the 1.10 stable branch, it failed on the python2 check 😞
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-763038995
> @madison-ookla Can you please rebase (or commit --amend followed by --force-push ? ) I think there were some CI woes when you pushed it last time and we did not have tests run for this one.
Done!
----------------------------------------------------------------
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] potiuk commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550278683
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
This is no problem. Just skip the test for py < 3.4. The upgradecheck is supposed to be run after people migrate to Python 3.6+. Something like that (or using six) should do.
```
@pytest.mark.skipif(sys.version_info < (3, 4), reason="requires python3.6 or higher")
```
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-756947378
Updated to retain support for python 2, only check python files, and accumulate exceptions as problems rather than re-raising exceptions!
----------------------------------------------------------------
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] madison-ookla commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r551533352
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
More than happy to add this to the problems report! My gut instinct for big projects like this is "preserve existing behavior by default", so I didn't want to change things up too much initially 😄 Also good to know on the macros! I can add that to the logic as well. Can they be python files that don't have the `.py` extension but are ostensibly python otherwise?
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r552602509
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
The `airflow upgrade_check` is intended to run on `1.10.X` version of Airflow so I'm afraid that it has to support Py2.7
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-770099964
Follow up PR here: https://github.com/apache/airflow/pull/13981
----------------------------------------------------------------
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] potiuk commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550279376
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
I think we should be completely safe to put skipIf < 3.6 on all upgrade check tests @turbaszek @dimberman ?
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r551149721
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Imho we should add this info to `problem` instead of rising the exception. In this way the command will work and users will be informed about all problems - including undesired files. WDYT?
----------------------------------------------------------------
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] madison-ookla commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550280411
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Gotcha! This was caught by the `Compile code using python2` pre-commit hook (run via breeze), which is what clued me in initially.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550284307
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Ah ok. Then if it a that stage, that might be a bit more complex, but just a bit. Simply exclude upgrade_check tests + upgrade_check sources in those two pre-commit checks (they already contain some exclusions): https://github.com/apache/airflow/blob/748d05f519c0e5427d2a46763591f03697c4d585/.pre-commit-config.yaml#L266
----------------------------------------------------------------
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] madison-ookla commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550288101
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Can do! I'll wait until some others chime in on whether or not this can be 3.6+ exclusively, then get to work on that if we get the 👍 🙂
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-752242570
I'm hoping I've done everything correctly...apologies if not! It's my first time contributing to airflow in quite some time 😄
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-764967795
Is this ready to be merged?
----------------------------------------------------------------
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] madison-ookla edited a comment on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla edited a comment on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-764967795
Is this ready okay be merged?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550278683
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
This is no problem. Just skip the test for py < 3.4. The upgradecheck is supposed to be run after people migrate to Python 3.6+. Something like that (or using six) should do.
```
@pytest.mark.skipif(sys.version_info < (3, 4), reason="requires python3.4 or higher")
```
----------------------------------------------------------------
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] potiuk commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-761808282
@madison-ookla Can you please rebase (or commit --amend followed by --force-push ? ) I think there were some CI woes when you pushed it last time and we did not have tests run for this one.
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-756947378
Updated to retain support for python 2, only check python files, and accumulate exceptions as problems rather than re-raising exceptions!
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-765757730
It might be my lack of understanding, but it seems the CI failures are unrelated?
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r551149721
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Imho we should add this info to `problem` instead of rising the exception. In this way the command will work and users will be informed about all problems - including undesired files. WDYT?
Btw. shouldn't we only check files with `.py` extension? Currently we open every file and I as far as I'm aware macros have to be python files not, `xlsx` for example 😄
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-769992004
> @madison-ookla This PR failed tests for Python 2.7 -- can you take a look please and fix it if possible in a separate PR -- thanks :)
>
> https://github.com/apache/airflow/runs/1770939841#step:6:821
Oh dear....yes, I'll look into this!
----------------------------------------------------------------
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] kaxil commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-765709737
Unfortunately, the tests were not running on v1-10-stable -- I just fixed it and rebased your PR -- 🤞 let's wait for CI results before I merge this one
----------------------------------------------------------------
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] kaxil merged pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13371:
URL: https://github.com/apache/airflow/pull/13371
----------------------------------------------------------------
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] kaxil commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-769985865
@madison-ookla This PR failed tests for Python 2.7 -- can you take a look please and fix it if possible in a separate PR -- thanks :)
----------------------------------------------------------------
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] potiuk commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550278781
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
+ file_path
Review comment:
SkipIf :) - see below.
----------------------------------------------------------------
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] madison-ookla commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550276117
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
I think this might run into the same [python 2.7](https://docs.python.org/2/library/importlib.html) issue as the [other comment you made](https://github.com/apache/airflow/pull/13371/files#r550270606) 😮 I'm not opposed to changing this logic, if you have an alternate suggestion! I think the issue I was running into had to deal with the fact that the file was binary, not that it was in a different encoding (as I imagine `open` would handle other string encodings appropriately...but admittedly I know less about that side of things).
----------------------------------------------------------------
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] kaxil edited a comment on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-769985865
@madison-ookla This PR failed tests for Python 2.7 -- can you take a look please and fix it if possible in a separate PR -- thanks :)
https://github.com/apache/airflow/runs/1770939841#step:6:821
----------------------------------------------------------------
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] madison-ookla commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r552858960
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Thanks @turbaszek! With that in mind @potiuk, I'll make sure this is python 2.7 compatible. I'm going to be changing it up a bit though to raise these issues as problems rather than an exception, per the comments below.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550270606
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
+ file_path
Review comment:
I think it would also be great to add the original exception as a "chained" exception https://docs.python.org/3/tutorial/errors.html#exception-chaining this way we can see also the original stack trace.
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
I think it would be much better to fix the problem as well for non-default encodings.
As of python 3.4 there is this function in the importlib:
https://docs.python.org/3/library/importlib.html?highlight=importlib#importlib.util.decode_source
And it will read the source code of python, respecting the encoding defined in the source code( the `# coding = ` comment). This way it will be more universal and handle the cases where non-utf8 encoding is used (which might be important in many countries).
I am not sure if similar approach is used in other rules - if so, the we can make a common approach and correct it in all those places.
----------------------------------------------------------------
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] kaxil commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-769994636
> > @madison-ookla This PR failed tests for Python 2.7 -- can you take a look please and fix it if possible in a separate PR -- thanks :)
> > https://github.com/apache/airflow/runs/1770939841#step:6:821
>
> Oh dear....yes, I'll look into this!
Thank You, appreciate it :)
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r551736426
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Plugins can only be python files with `.py` extension, otherwise we skip them:
https://github.com/apache/airflow/blob/748d05f519c0e5427d2a46763591f03697c4d585/airflow/plugins_manager.py#L200-L201
----------------------------------------------------------------
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] madison-ookla commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-764967795
Is this ready to be merged?
----------------------------------------------------------------
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] madison-ookla edited a comment on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla edited a comment on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-764967795
Is this ready okay be merged?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r550278683
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
This is no problem. Just skip the test for non-py2.7. The upgradecheck is supposed to be run after people migrate to Python 3.6+. Something like that (or using six) should do.
```
@pytest.mark.skipif(sys.version_info < (3, 6), reason="requires python3.6 or higher")
```
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#issuecomment-761808195
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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] madison-ookla commented on a change in pull request #13371: Add clearer exception for read failures in macro plugin upgrade check
Posted by GitBox <gi...@apache.org>.
madison-ookla commented on a change in pull request #13371:
URL: https://github.com/apache/airflow/pull/13371#discussion_r552162168
##########
File path: airflow/upgrade/rules/airflow_macro_plugin_removed.py
##########
@@ -39,9 +39,16 @@ def _check_file(self, file_path):
problems = []
class_name_to_check = self.MACRO_PLUGIN_CLASS.split(".")[-1]
with open(file_path, "r") as file_pointer:
- for line_number, line in enumerate(file_pointer, 1):
- if class_name_to_check in line:
- problems.append(self._change_info(file_path, line_number))
+ try:
+ for line_number, line in enumerate(file_pointer, 1):
+ if class_name_to_check in line:
+ problems.append(self._change_info(file_path, line_number))
+ except UnicodeDecodeError:
+ raise Exception(
+ "Unable to read file {}, it may need to be added to the .airflowignore".format(
Review comment:
Just want to check back in on this @potiuk @turbaszek @dimberman - we can make the upgrade check Python 3.6+ (or even just the tests Py3.6+ if we'd like), but currently the package still supports 2.7 (https://github.com/apache/airflow/blob/v1-10-stable/airflow/upgrade/setup.cfg#L35). Should we bump up these requirements to 3.6? I agree that it makes sense to do so, since this is a utility for upgrading to Airflow 2.0 which is 3.6+.
----------------------------------------------------------------
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