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/06/22 16:06:00 UTC

[GitHub] [airflow] VinayGb665 opened a new pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

VinayGb665 opened a new pull request #9477:
URL: https://github.com/apache/airflow/pull/9477


   
   ---
   Adding a yaml file parser to LocalFilesystemBackend, structure would be similar to that of earlier JSON found [here](https://github.com/apache/airflow/blob/master/docs/howto/use-alternative-secrets-backend.rst#storing-and-retrieving-connections) and [here](https://github.com/apache/airflow/blob/master/docs/howto/use-alternative-secrets-backend.rst#storing-and-retrieving-variables)
   
   [Github issue](https://github.com/apache/airflow/issues/9417)
   
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x] Description above provides context of the change
   - [x] Unit tests coverage for changes (not needed for documentation changes)
   - [x] Target Github ISSUE in description if exists
   - [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [x] Relevant documentation is updated including usage instructions.
   - [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   ---
   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).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   


----------------------------------------------------------------
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 merged pull request #9477: YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #9477:
URL: https://github.com/apache/airflow/pull/9477


   


----------------------------------------------------------------
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 #9477: [WIP] YAML file format in LocalFilesystemBackend

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


   Is this still a WIP? Should I go further?


----------------------------------------------------------------
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 #9477: YAML file format in LocalFilesystemBackend

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


   Please create new PR :-) 


----------------------------------------------------------------
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] VinayGb665 commented on pull request #9477: YAML file format in LocalFilesystemBackend

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


   Looking forward to contribute more.
   If you have any tickets in mind please suggest :smile: .


----------------------------------------------------------------
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 #9477: YAML file format in LocalFilesystemBackend

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


   @VinayGb665 Thank you very much. What next? 🐈


----------------------------------------------------------------
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 a change in pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9477:
URL: https://github.com/apache/airflow/pull/9477#discussion_r444539645



##########
File path: tests/secrets/test_local_filesystem.py
##########
@@ -104,6 +105,19 @@ def test_missing_file(self, mock_exists):
         ):
             local_filesystem.load_variables("a.json")
 
+    @parameterized.expand(
+        (
+            ({}, {}),
+            ({"KEY": "AAA"}, {"KEY": "AAA"}),
+            ({"KEY_A": "AAA", "KEY_B": "BBB"}, {"KEY_A": "AAA", "KEY_B": "BBB"}),

Review comment:
       Can you use YAML file here? 




----------------------------------------------------------------
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] VinayGb665 edited a comment on pull request #9477: YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
VinayGb665 edited a comment on pull request #9477:
URL: https://github.com/apache/airflow/pull/9477#issuecomment-650117066


   > Could you add support for the extra parameter without using nested JSON?
   > I am currently thinking about something like this.
   > 
   > ```yaml
   > conn_c:
   >    conn_type: scheme
   >    host: host
   >    extra_dejson:
   >        extra__google_cloud_platform__keyfile_dict: AAA
   > ```
   > 
   > Currently, the extra parameter can only be defined as follows.
   > 
   > ```yaml
   > conn_c:
   >    conn_type: scheme
   >    host: host
   >    extra_dejson: {"extra__google_cloud_platform__keyfile_dict": "file_content"}
   > ```
   > 
   > It works, but it's not perfect.
   
   
   AAA being the file name?


----------------------------------------------------------------
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] VinayGb665 commented on pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

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


   > Is this still a WIP? Should I go further?
   
   Nope not a WIP. Ill update the title. Please you can go further


----------------------------------------------------------------
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 #9477: YAML file format in LocalFilesystemBackend

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


   @VinayGb665 The content of the extra field depends on the implementation of the specific hook.
   https://airflow.readthedocs.io/en/latest/howto/connection/gcp.html
   
   For GCP, we have the following keys:
   
   > - extra__google_cloud_platform__project - Project Id
   > - extra__google_cloud_platform__key_path - Keyfile Path
   > - extra__google_cloud_platform__keyfile_dict - Keyfile JSON
   > - extra__google_cloud_platform__scope - Scopes
   > - extra__google_cloud_platform__num_retries - Number of Retries
   
   https://airflow.readthedocs.io/en/latest/howto/connection/gcp.html
   
   From a technical point of view, an extra is a JSON object with any keys and any values.


----------------------------------------------------------------
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 a change in pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9477:
URL: https://github.com/apache/airflow/pull/9477#discussion_r444539545



##########
File path: docs/howto/use-alternative-secrets-backend.rst
##########
@@ -116,6 +116,28 @@ The following is a sample JSON file.
         }
     }
 
+The YAML file structure is similar to that of a JSON. The key-value pair of connection ID and the definitions of one or more connections.
+The connection can be defined as a URI (string) or JSON object.
+For a guide about defining a connection as a URI, see:: :ref:`generating_connection_uri`.
+For a description of the connection object parameters see :class:`~airflow.models.connection.Connection`.
+The following is a sample YAML file.
+
+.. code-block:: yaml
+
+    CONN_A: 'mysq://host_a'

Review comment:
       Can you use these examples in tests?  This will facilitate verification that everything is okay.




----------------------------------------------------------------
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] VinayGb665 commented on a change in pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

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



##########
File path: airflow/secrets/local_filesystem.py
##########
@@ -84,6 +86,29 @@ def _parse_env_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSynt
     return secrets, errors
 
 
+def _parse_yaml_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSyntaxError]]:
+    """
+    Parse a file in the YAML format.
+
+    :param file_path: The location of the file that will be processed.
+    :type file_path: str
+    :return: Tuple with mapping of key and list of values and list of syntax errors
+    """
+    with open(file_path) as f:
+        content = f.read()
+
+    if not content:
+        return {}, [FileSyntaxError(line_no=1, message="The file is empty.")]
+    try:
+        secrets = yaml.safe_load(content)
+    except yaml.YAMLError as e:

Review comment:
       Fixed




----------------------------------------------------------------
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] VinayGb665 commented on pull request #9477: YAML file format in LocalFilesystemBackend

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


   Oh yes. I got the issue. 
   Should i reopen the PR or create a new 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] VinayGb665 commented on pull request #9477: YAML file format in LocalFilesystemBackend

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


   @mik-laj If the intended YAML would look something like this then it should work fine with the current implementation itself
   ```
   extra_dejson:
     extra__google_cloud_platform__keyfile_dict:
       a: b
       c: d
     extra__google_cloud_platform__num_retries: 1
   ```


----------------------------------------------------------------
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 #9477: [WIP] YAML file format in LocalFilesystemBackend

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


   Can you also update docs?


----------------------------------------------------------------
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 #9477: YAML file format in LocalFilesystemBackend

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


   When I tried to define the connection in this way, I saw the message below.
   ```
   airflow.exceptions.AirflowException: The object have illegal keys: extra_dejson. The dictionary can only contain the following keys: {'host', 'port', 'schema', 'extra', 'login', 'conn_type', 'password', 'uri', 'conn_id'}
   
   ```


----------------------------------------------------------------
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] VinayGb665 commented on a change in pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

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



##########
File path: tests/secrets/test_local_filesystem.py
##########
@@ -104,6 +105,19 @@ def test_missing_file(self, mock_exists):
         ):
             local_filesystem.load_variables("a.json")
 
+    @parameterized.expand(
+        (
+            ({}, {}),
+            ({"KEY": "AAA"}, {"KEY": "AAA"}),
+            ({"KEY_A": "AAA", "KEY_B": "BBB"}, {"KEY_A": "AAA", "KEY_B": "BBB"}),

Review comment:
       Added  YAML files for both variables and connection tests




----------------------------------------------------------------
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 #9477: YAML file format in LocalFilesystemBackend

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


   Could you add support for the extra parameter without using nested JSON?
   I am currently thinking about something like this.
   ```
   conn_c:
      conn_type: scheme
      host: host
      extra_dejson:
          extra__google_cloud_platform__keyfile_dict: AAA
   ```
   Currently, the extra parameter can only be defined as follows.
   ```
   conn_c:
      conn_type: scheme
      host: host
      extra_dejson: {"extra__google_cloud_platform__keyfile_dict": "file_content"}
   ```
   It works, but it's not perfect.


----------------------------------------------------------------
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 pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #9477:
URL: https://github.com/apache/airflow/pull/9477#issuecomment-647618294


   Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
   Here are some useful points:
   - Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
   - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
   - Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
   - Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
   - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
   - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
   Apache Airflow is a community-driven project and together we are making it better πŸš€.
   In case of doubts contact the developers at:
   Mailing List: dev@airflow.apache.org
   Slack: https://apache-airflow-slack.herokuapp.com/
   


----------------------------------------------------------------
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 a change in pull request #9477: [WIP] YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9477:
URL: https://github.com/apache/airflow/pull/9477#discussion_r444542716



##########
File path: airflow/secrets/local_filesystem.py
##########
@@ -84,6 +86,29 @@ def _parse_env_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSynt
     return secrets, errors
 
 
+def _parse_yaml_file(file_path: str) -> Tuple[Dict[str, List[str]], List[FileSyntaxError]]:
+    """
+    Parse a file in the YAML format.
+
+    :param file_path: The location of the file that will be processed.
+    :type file_path: str
+    :return: Tuple with mapping of key and list of values and list of syntax errors
+    """
+    with open(file_path) as f:
+        content = f.read()
+
+    if not content:
+        return {}, [FileSyntaxError(line_no=1, message="The file is empty.")]
+    try:
+        secrets = yaml.safe_load(content)
+    except yaml.YAMLError as e:

Review comment:
       ```suggestion
       except yaml.MarkedYAMLError as e:
           return {}, [FileSyntaxError(line_no=e.context_mark.line, message=str(e))]
       except yaml.YAMLError as e:
   ```




----------------------------------------------------------------
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 edited a comment on pull request #9477: YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #9477:
URL: https://github.com/apache/airflow/pull/9477#issuecomment-650107155


   Could you add support for the extra parameter without using nested JSON?
   I am currently thinking about something like this.
   ```yaml
   conn_c:
      conn_type: scheme
      host: host
      extra_dejson:
          extra__google_cloud_platform__keyfile_dict: AAA
   ```
   Currently, the extra parameter can only be defined as follows.
   ```yaml
   conn_c:
      conn_type: scheme
      host: host
      extra_dejson: {"extra__google_cloud_platform__keyfile_dict": "file_content"}
   ```
   It works, but it's not perfect.


----------------------------------------------------------------
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] VinayGb665 commented on pull request #9477: YAML file format in LocalFilesystemBackend

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


   > Could you add support for the extra parameter without using nested JSON?
   > I am currently thinking about something like this.
   > 
   > ```yaml
   > conn_c:
   >    conn_type: scheme
   >    host: host
   >    extra_dejson:
   >        extra__google_cloud_platform__keyfile_dict: AAA
   > ```
   > 
   > Currently, the extra parameter can only be defined as follows.
   > 
   > ```yaml
   > conn_c:
   >    conn_type: scheme
   >    host: host
   >    extra_dejson: {"extra__google_cloud_platform__keyfile_dict": "file_content"}
   > ```
   > 
   > It works, but it's not perfect.
   
   
   
   > Could you add support for the extra parameter without using nested JSON?
   > I am currently thinking about something like this.
   > 
   > ```yaml
   > conn_c:
   >    conn_type: scheme
   >    host: host
   >    extra_dejson:
   >        extra__google_cloud_platform__keyfile_dict: AAA
   > ```
   > 
   > Currently, the extra parameter can only be defined as follows.
   > 
   > ```yaml
   > conn_c:
   >    conn_type: scheme
   >    host: host
   >    extra_dejson: {"extra__google_cloud_platform__keyfile_dict": "file_content"}
   > ```
   > 
   > It works, but it's not perfect.
   
   AAA being the file name?


----------------------------------------------------------------
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 pull request #9477: YAML file format in LocalFilesystemBackend

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #9477:
URL: https://github.com/apache/airflow/pull/9477#issuecomment-650092949


   Awesome work, congrats on your first merged pull request!
   


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