[airflow] branch v1-10-stable updated: Add clearer exception for read failures in macro plugin upgrade check (#13371)

[ka...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

kaxilnaik pushed a commit to branch v1-10-stable
in repository https://gitbox.apache.org/repos/asf/airflow.git


The following commit(s) were added to refs/heads/v1-10-stable by this push:
     new 3de7036  Add clearer exception for read failures in macro plugin upgrade check (#13371)
3de7036 is described below

commit 3de7036550b20a2964352c2e1f9c4a2e5550050a
Author: Madison Bowden <52...@users.noreply.github.com>
AuthorDate: Fri Jan 22 16:04:07 2021 -0800

    Add clearer exception for read failures in macro plugin upgrade check (#13371)
    
    Resolves #13349
---
 .../upgrade/rules/airflow_macro_plugin_removed.py  | 11 ++++--
 .../rules/test_airflow_macro_plugin_removed.py     | 45 +++++++++++++++++-----
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/airflow/upgrade/rules/airflow_macro_plugin_removed.py b/airflow/upgrade/rules/airflow_macro_plugin_removed.py
index ca4f546..bb2da42 100644
--- a/airflow/upgrade/rules/airflow_macro_plugin_removed.py
+++ b/airflow/upgrade/rules/airflow_macro_plugin_removed.py
@@ -39,9 +39,12 @@ class AirflowMacroPluginRemovedRule(BaseRule):
         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:
+                problems.append("Unable to read python file {}".format(file_path))
         return problems
 
     def check(self):
@@ -49,5 +52,7 @@ class AirflowMacroPluginRemovedRule(BaseRule):
         file_paths = list_py_file_paths(directory=dag_folder, include_examples=False)
         problems = []
         for file_path in file_paths:
+            if not file_path.endswith(".py"):
+                continue
             problems.extend(self._check_file(file_path))
         return problems
diff --git a/tests/upgrade/rules/test_airflow_macro_plugin_removed.py b/tests/upgrade/rules/test_airflow_macro_plugin_removed.py
index 2d7b7ba..bbf18ce 100644
--- a/tests/upgrade/rules/test_airflow_macro_plugin_removed.py
+++ b/tests/upgrade/rules/test_airflow_macro_plugin_removed.py
@@ -20,12 +20,14 @@ from unittest import TestCase
 from tempfile import NamedTemporaryFile
 from tests.compat import mock
 
-from airflow.upgrade.rules.airflow_macro_plugin_removed import AirflowMacroPluginRemovedRule
+from airflow.upgrade.rules.airflow_macro_plugin_removed import (
+    AirflowMacroPluginRemovedRule,
+)
 
 
 @contextmanager
-def create_temp_file(mock_list_files, lines):
-    with NamedTemporaryFile("w+") as temp_file:
+def create_temp_file(mock_list_files, lines, extension=".py"):
+    with NamedTemporaryFile("w+", suffix=extension) as temp_file:
         mock_list_files.return_value = [temp_file.name]
         temp_file.writelines("\n".join(lines))
         temp_file.flush()
@@ -34,13 +36,16 @@ def create_temp_file(mock_list_files, lines):
 
 @mock.patch("airflow.upgrade.rules.airflow_macro_plugin_removed.list_py_file_paths")
 class TestAirflowMacroPluginRemovedRule(TestCase):
+
+    def test_rule_info(self, mock_list_files):
+        rule = AirflowMacroPluginRemovedRule()
+        assert isinstance(rule.description, str)
+        assert isinstance(rule.title, str)
+
     def test_valid_check(self, mock_list_files):
         lines = ["import foo.bar.baz"]
         with create_temp_file(mock_list_files, lines):
             rule = AirflowMacroPluginRemovedRule()
-            assert isinstance(rule.description, str)
-            assert isinstance(rule.title, str)
-
             msgs = rule.check()
             assert 0 == len(msgs)
 
@@ -52,10 +57,6 @@ class TestAirflowMacroPluginRemovedRule(TestCase):
         with create_temp_file(mock_list_files, lines) as temp_file:
 
             rule = AirflowMacroPluginRemovedRule()
-
-            assert isinstance(rule.description, str)
-            assert isinstance(rule.title, str)
-
             msgs = rule.check()
             assert 2 == len(msgs)
 
@@ -66,3 +67,27 @@ class TestAirflowMacroPluginRemovedRule(TestCase):
                 "{} (line {})".format(base_message, line_number) for line_number in [1, 2]
             ]
             assert expected_messages == msgs
+
+    def test_non_python_file_ignored(self, mock_list_files):
+        lines = [
+            "import airflow.AirflowMacroPlugin",
+            "from airflow import AirflowMacroPlugin",
+        ]
+        with create_temp_file(mock_list_files, lines, extension=".other"):
+            # Although this file "matches", it shouldn't be flagged because
+            # only python files are checked for macros anyway
+            rule = AirflowMacroPluginRemovedRule()
+            msgs = rule.check()
+            assert 0 == len(msgs)
+
+    def test_bad_file_failure(self, mock_list_files):
+        # Write a binary file
+        with NamedTemporaryFile("wb+", suffix=".py") as temp_file:
+            mock_list_files.return_value = [temp_file.name]
+            temp_file.write(b"{\x03\xff\x00d")
+            temp_file.flush()
+
+            rule = AirflowMacroPluginRemovedRule()
+            msgs = rule.check()
+            assert 1 == len(msgs)
+            assert msgs == ["Unable to read python file {}".format(temp_file.name)]