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/11/10 15:28:58 UTC

[GitHub] [airflow] turbaszek opened a new pull request #12252: Remove providers imports from core examples

turbaszek opened a new pull request #12252:
URL: https://github.com/apache/airflow/pull/12252


   Core example DAGs should not depend on any non-core dependency
   like providers packages.
   
   I also added a pre-commit check to avoid regression:
   ```
   ➜ pre-commit run no-providers-in-core-examples --all-files
   No providers imports in core example DAGs................................Failed
   - hook id: no-providers-in-core-examples
   - exit code: 1
   
   airflow/example_dags/example_dag_decorator.py:25:from airflow.providers.http.operators.http import SimpleHttpOperator
   ```
   
   closes: #12247
   ---
   **^ 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] XD-DENG commented on a change in pull request #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520761008



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow

Review comment:
       ```suggestion
           description: The core example DAGs have no dependencies other than core Airflow
   ```




----------------------------------------------------------------
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 #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#issuecomment-724847569


   The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run 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] mik-laj commented on pull request #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#issuecomment-724841113


   @turbaszek There is no reason. I've tried to move providers as many as possible, but we can move it back to the core.


----------------------------------------------------------------
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 #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520729028



##########
File path: airflow/operators/python.py
##########
@@ -145,6 +146,8 @@ class _PythonDecoratedOperator(BaseOperator):
     """
 
     template_fields = ('op_args', 'op_kwargs')
+    template_fields_renderers = {"op_args": "py", "op_kwargs": "py"}

Review comment:
       I hope no one will be angry that I added this in this PR 😄 
   
   <img width="737" alt="Screenshot 2020-11-10 at 18 10 38" src="https://user-images.githubusercontent.com/9528307/98707216-1466ce80-2380-11eb-8b00-c01fb8f11494.png">
   




----------------------------------------------------------------
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 merged pull request #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #12252:
URL: https://github.com/apache/airflow/pull/12252


   


----------------------------------------------------------------
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 #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520768793



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow
+        entry: "^\\s*from airflow.providers.*"

Review comment:
       But this means that the `.` between should be escaped 👀 




----------------------------------------------------------------
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 #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520766946



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow
+        entry: "^\\s*from airflow.providers.*"

Review comment:
       Yes it works:
   ```
   ➜ pre-commit run no-providers-in-core-examples --all-files
   No providers imports in core example DAGs................................Failed
   - hook id: no-providers-in-core-examples
   - exit code: 1
   
   airflow/example_dags/example_complex.py:29:from airflow.providers import google
   ```




----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520768814



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow
+        entry: "^\\s*from airflow.providers.*"

Review comment:
       Cool. Thanks for the clarification!




----------------------------------------------------------------
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 #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520768230



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow
+        entry: "^\\s*from airflow.providers.*"

Review comment:
       The `*` is not glob pattern but a regexp definition `.*`




----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520762065



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow
+        entry: "^\\s*from airflow.providers.*"

Review comment:
       Does this pattern cover cases like `from airflow.providers import ...`? Seems not to me




----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520769503



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow
+        entry: "^\\s*from airflow.providers.*"

Review comment:
       Haha, gotcha either here or there




----------------------------------------------------------------
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 #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520767390



##########
File path: .pre-commit-config.yaml
##########
@@ -294,6 +294,13 @@ repos:
         entry: "\\|\\s*safe"
         files: \.html$
         pass_filenames: true
+      - id: no-providers-in-core-examples
+        language: pygrep
+        name: No providers imports in core example DAGs
+        description: The core example DAGs have to no dependencies other than core Airflow
+        entry: "^\\s*from airflow.providers.*"

Review comment:
       Works also for not top level imports:
   ```
   No providers imports in core example DAGs................................Failed
   - hook id: no-providers-in-core-examples
   - exit code: 1
   
   airflow/example_dags/example_complex.py:35:    from airflow.providers import google
   ```




----------------------------------------------------------------
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 pull request #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#issuecomment-724779794


   @mik-laj @potiuk is there any particular reason why http package is released as provider? It uses only
   ```
   import requests
   import tenacity
   ```
   which are always installed with Airflow 🤔 


----------------------------------------------------------------
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 #12252: Remove providers imports from core examples

Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12252:
URL: https://github.com/apache/airflow/pull/12252#discussion_r520652376



##########
File path: .pre-commit-config.yaml
##########
@@ -394,12 +401,6 @@ repos:
         pass_filenames: false
         require_serial: true
         additional_dependencies: ['pyyaml']
-      - id: pre-commit-descriptions
-        name: Check if pre-commits are described
-        entry: ./scripts/ci/pre_commit/pre_commit_check_pre_commits.sh
-        language: system
-        files: ^.pre-commit-config.yaml$|^STATIC_CODE_CHECKS.rst|^breeze-complete$
-        require_serial: true

Review comment:
       Removed this as it was duplicated:
   https://github.com/apache/airflow/blob/3ddf4d0c4501530f31f6dadb69fcb461acf24387/.pre-commit-config.yaml#L397-L402
   https://github.com/apache/airflow/blob/3ddf4d0c4501530f31f6dadb69fcb461acf24387/.pre-commit-config.yaml#L459-L464




----------------------------------------------------------------
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