Repository: apache/airflow
Pull request: Add conf not importable from airflow rule
This rule checks to ensure user properly imports "conf" to be compatible with Airflow 2.0
closes: https://github.com/apache/airflow/issues/13945 


<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.

Feel free to ping committers for the review!

In case of existing issue, reference it using one of the following:

closes: #ISSUE
related: #ISSUE

How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->

---
**^ 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).


Problem statement:
Add conf not importable from airflow rule
**Description**

A new rule should be added to Airflow upgrade check to verify that users import `conf` from proper place and their code will work with 2.0.

**Use case / motivation**

In 1.10 users can do:

```py
In [1]: from airflow import conf

In [2]: conf
Out[2]: <airflow.configuration.AirflowConfigParser at 0x7f6f28650f28>
```
in 2.0 it's not possible:
```py
In [1]: from airflow import conf
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-1-2cf28b1802b4> in <module>
----> 1 from airflow import conf

ImportError: cannot import name 'conf'
```

**Are you willing to submit a PR?**

Possibly if time allows 🕐 

**Related Issues**

N/A

Hints from issue discussion:
Hey @turbaszek, I am interested in making a PR for this. 

I see you tagged this with good first issue. I am still a novice when it comes to airflow, so if you could help me with the review I think we can accomplish this together.
hey @turbaszek, struggling to get started with this PR. Could you elaborate more on "Airflow upgrade check", not sure about the implementation strategy at the moment?
> Could you elaborate more on "Airflow upgrade check", not sure about the implementation strategy at the moment?

@griffincosgrove upgrade check is additional package that can be installed next to Airflow 1.10.X and allows users check if they are ready to upgrade to Airflow 2.0:
https://github.com/apache/airflow/tree/v1-10-stable/airflow/upgrade

As you can see solving this issue requires to open a PR to `v1-10-stable` branch not `master`. What we need is a new rule that will check that users do not use `from airflow import conf`. Here are existing rules for reference:
https://github.com/apache/airflow/tree/v1-10-stable/airflow/upgrade/rules

Hi @turbaszek is @griffincosgrove working on this issue already, if not I will like to take it up. I am new to Airflow as well and looking for a good-first-issue to taken on. Could this be assigned to me if @griffincosgrove is occupied at the moment? Thanks
@digitalsimboja you can work on as @griffincosgrove seems to be inactive for last few days 
Had trouble with this one. Good luck @digitalsimboja!
@turbaszek  Thanks I am taking it on already. Do I need to install anything in order to access the file. I have looking through the repo I have but yet to locate the file at **apache/airflow/tree/v1-10-stable/airflow/upgrade**. Need to mention that I run Airflow on WSL. 
@digitalsimboja you need to checkout to `v1-10-stable` branch, then you should have the `upgrade` directory.

Commit message: Add conf not importable from airflow rule #13945

Patch to review:
diff --git a/airflow/upgrade/rules/fix_conf_not_importable_from_airflow.py b/airflow/upgrade/rules/fix_conf_not_importable_from_airflow.py
new file mode 100644
index 0000000000000..0ed2037eb732c
--- /dev/null
+++ b/airflow/upgrade/rules/fix_conf_not_importable_from_airflow.py
@@ -0,0 +1,64 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.upgrade.rules.base_rule import BaseRule
+from airflow import conf
+from airflow.utils.dag_processing import list_py_file_paths
+
+
+class ProperlyImportConfFromAirflow(BaseRule):
+    """
+      ProperlyImportConfFromAirflow class to ensure proper import of conf to work in Airflow 2.0
+      """
+    title = "Ensure Users Properly Import conf from Airflow"
+    description = """\
+    In Airflow-2.0, it's not possible to import `conf` from airflow by using `import conf from airflow`
+    To ensure your code works in Airflow 2.0, you should use `from airflow.configuration import conf`.
+                      """
+
+    wrong_conf_import = "from airflow import conf"
+    proper_conf_import = "from airflow.configuration import conf"
+
+    def _conf_import_info(self, file_path, line_number):
+        return "Warning: In Airflow 2.0, it's not possible to import `conf` by using `{}`. " \
+               "To ensure your code works in Airflow 2.0, you should use `{}`." \
+               " Affected file: {} (line {})".format(
+                self.wrong_conf_import, self.proper_conf_import, file_path, line_number
+                )
+
+    def _check_file(self, file_path):
+        problems = []
+        conf_import_check = self.wrong_conf_import
+        with open(file_path, "r") as file_pointer:
+            try:
+                for line_number, line in enumerate(file_pointer, 1):
+                    if conf_import_check in line:
+                        problems.append(self._conf_import_info(file_path, line_number))
+            except UnicodeDecodeError:
+                problems.append("Unable to read python file {}".format(file_path))
+        return problems
+
+    def check(self):
+        dag_folder = conf.get("core", "dags_folder")
+        files = list_py_file_paths(directory=dag_folder, include_examples=False)
+        problems = []
+        for file in files:
+            if not file.endswith(".py"):
+                continue
+            problems.extend(self._check_file(file))
+        return problems
diff --git a/tests/upgrade/rules/test_fix_conf_not_importable_from_airflow.py b/tests/upgrade/rules/test_fix_conf_not_importable_from_airflow.py
new file mode 100644
index 0000000000000..e1e009a6adb80
--- /dev/null
+++ b/tests/upgrade/rules/test_fix_conf_not_importable_from_airflow.py
@@ -0,0 +1,82 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from contextlib import contextmanager
+from tempfile import NamedTemporaryFile
+from unittest import TestCase
+
+from tests.compat import mock
+
+from airflow.upgrade.rules.fix_conf_not_importable_from_airflow import ProperlyImportConfFromAirflow
+
+
+@contextmanager
+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()
+        yield temp_file
+
+
+@mock.patch("airflow.upgrade.rules.fix_conf_not_importable_from_airflow.list_py_file_paths")
+class TestProperConfImportFromAirflow(TestCase):
+
+    def test_check_conf_import_info(self, mock_list_files):
+        rule = ProperlyImportConfFromAirflow()
+        assert isinstance(rule.title, str)
+        assert isinstance(rule.description, str)
+
+    def test_valid_check(self, mock_list_files):
+        lines = ["import foo.bar.baz"]
+        with create_temp_file(mock_list_files, lines):
+            rule = ProperlyImportConfFromAirflow()
+            msgs = rule.check()
+            assert 0 == len(msgs)
+
+    def test_invalid_check(self, mock_list_files):
+        lines = [
+            "from airflow import conf",
+            "from airflow import conf",
+        ]
+        with create_temp_file(mock_list_files, lines) as temp_file:
+            rule = ProperlyImportConfFromAirflow()
+            msgs = rule.check()
+            assert 2 == len(msgs)
+
+            base_message = "Warning: In Airflow 2.0, it's not possible to import `conf` " \
+                           "by using `from airflow import conf`. " \
+                           "To ensure your code works in Airflow 2.0, " \
+                           "you should use `from airflow.configuration import conf`. " \
+                           "Affected file: {}".format(temp_file.name)
+            expected_messages = [
+                "{} (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 = [
+            "from airflow import conf",
+            "from airflow import conf",
+        ]
+        with create_temp_file(mock_list_files, lines, extension=".etc"):
+            # Although this file "matches", it shouldn't be flagged because
+            # only python files are checked for dags anyway
+            rule = ProperlyImportConfFromAirflow()
+            msgs = rule.check()
+            assert 0 == len(msgs)


Code review comments:
diff_hunk: @@ -0,0 +1,64 @@
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.upgrade.rules.base_rule import BaseRule
+from airflow import conf
+from airflow.utils.dag_processing import list_py_file_paths
+
+
+class ProperlyImportConfFromAirflow(BaseRule):
+    """
+      ProperlyImportConfFromAirflow class to ensure proper import of conf to work in Airflow 2.0
+      """
+    title = "Ensure Users Properly Import conf from Airflow"
+    description = """\
+    In Airflow-2.0, it's not possible to import `conf` from airflow by using `import conf from airflow`
+    To ensure your code works in Airflow 2.0, you should use `from airflow.configuration import conf`.
+                      """
+
+    wrong_conf_import = "from airflow import conf"
+    proper_conf_import = "from airflow.configuration import conf"
+
+    def _conf_import_info(self, file_path, line_number):
+        return "Warning: In Airflow 2.0, it's not possible to import `conf` by using `{}`. " \
+               "To ensure your code works in Airflow 2.0, you should use `{}`." \
+               " Affected file: {} (line {})".format(
+                self.wrong_conf_import, self.proper_conf_import, file_path, line_number
+                )
path: airflow/upgrade/rules/fix_conf_not_importable_from_airflow.py
text: @user1:
```suggestion
        return "Affected file: {} (line {})".format(
                self.wrong_conf_import, self.proper_conf_import, file_path, line_number
                )
```
All else will be displayed anyway as it is in rule description, right?

@author:
Yes that is correct. I will go ahead and return only the affected file and line in this section. Thanks