YAML file format in LocalFilesystemBackend #9477

VinayGb665 · 2020-06-22T16:05:51Z

Adding a yaml file parser to LocalFilesystemBackend, structure would be similar to that of earlier JSON found here and here

Closes : #9417

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.
@wip ready for review

boring-cyborg · 2020-06-22T16:05:54Z

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 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 Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://apache-airflow-slack.herokuapp.com/

mik-laj · 2020-06-22T16:39:50Z

Can you also update docs?

mik-laj · 2020-06-23T22:17:26Z

docs/howto/use-alternative-secrets-backend.rst

+
+.. code-block:: yaml
+
+    CONN_A: 'mysq://host_a'


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

mik-laj · 2020-06-23T22:17:42Z

tests/secrets/test_local_filesystem.py

+        (
+            ({}, {}),
+            ({"KEY": "AAA"}, {"KEY": "AAA"}),
+            ({"KEY_A": "AAA", "KEY_B": "BBB"}, {"KEY_A": "AAA", "KEY_B": "BBB"}),


Can you use YAML file here?

Added YAML files for both variables and connection tests

mik-laj · 2020-06-23T22:26:38Z

airflow/secrets/local_filesystem.py

+        return {}, [FileSyntaxError(line_no=1, message="The file is empty.")]
+    try:
+        secrets = yaml.safe_load(content)
+    except yaml.YAMLError as e:


Suggested change

except yaml.YAMLError as e:

except yaml.MarkedYAMLError as e:

return {}, [FileSyntaxError(line_no=e.context_mark.line, message=str(e))]

except yaml.YAMLError as e:

mik-laj

I really like this change. Editing JSON files was tiring.

mik-laj · 2020-06-25T20:13:05Z

Is this still a WIP? Should I go further?

VinayGb665 · 2020-06-26T04:26:03Z

Is this still a WIP? Should I go further?

Nope not a WIP. Ill update the title. Please you can go further

boring-cyborg · 2020-06-26T09:50:00Z

Awesome work, congrats on your first merged pull request!

mik-laj · 2020-06-26T09:50:53Z

@VinayGb665 Thank you very much. What next? 🐈

VinayGb665 · 2020-06-26T10:01:08Z

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

mik-laj · 2020-06-26T10:25:15Z

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.

VinayGb665 · 2020-06-26T10:52:40Z

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.

AAA being the file name?

mik-laj · 2020-06-26T10:57:29Z

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

VinayGb665 · 2020-06-26T11:29:30Z

@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

mik-laj · 2020-06-26T12:09:08Z

Please create new PR :-)

- Support towards YAML is added in PR apache#9477 Most docs were updated for this. But a few docstrings and exception logging were missed - Fix a minor error in the doc .rst file.

- Support towards YAML is added in PR #9477 Most docs were updated for this. But a few docstrings and exception logging were missed

- Support towards YAML is added in PR apache/airflow#9477 Most docs were updated for this. But a few docstrings and exception logging were missed GitOrigin-RevId: c6467ba12d4a94027137e3173097d73be56c5d12

Vinay added 2 commits June 22, 2020 20:55

Adding YAML file parser

bc6ba66

Adding unit tests

67d95f4

boring-cyborg bot added the area:secrets label Jun 22, 2020

Vinay added 4 commits June 23, 2020 01:56

FIxing linting errors

202834e

Fixing exception handler

3648f74

Updating documentation

6ad497a

Running static-checks

2ebbfd6

VinayGb665 changed the title ~~[WIP] YAML file format in LocalFilesystemBackend~~ YAML file format in LocalFilesystemBackend Jun 23, 2020

VinayGb665 changed the title ~~YAML file format in LocalFilesystemBackend~~ [WIP] YAML file format in LocalFilesystemBackend Jun 23, 2020

mik-laj reviewed Jun 23, 2020

View reviewed changes

Vinay added 2 commits June 24, 2020 19:06

Adding unit tests with YAML

75255f7

Updating YAML error handling

afbc2b4

VinayGb665 requested a review from mik-laj June 25, 2020 15:07

mik-laj changed the title ~~[WIP] YAML file format in LocalFilesystemBackend~~ YAML file format in LocalFilesystemBackend Jun 26, 2020

mik-laj approved these changes Jun 26, 2020

View reviewed changes

mik-laj merged commit e7dff68 into apache:master Jun 26, 2020

This was referenced Jun 27, 2020

YAML file supporting extra json parameters for LocalFileSystemBackend #9546

Closed

YAML file supports extra json parameters #9549

Merged

XD-DENG mentioned this pull request Nov 24, 2020

Update logging & doc for LocalFilesystem Secrets Backend #12597

Merged

kaxil pushed a commit that referenced this pull request Nov 25, 2020

Update logging & doc for LocalFilesystem Secrets Backend (#12597)

c6467ba

- Support towards YAML is added in PR #9477 Most docs were updated for this. But a few docstrings and exception logging were missed

YAML file format in LocalFilesystemBackend #9477

YAML file format in LocalFilesystemBackend #9477

Uh oh!

Conversation

VinayGb665 commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented Jun 22, 2020

Uh oh!

mik-laj commented Jun 22, 2020

Uh oh!

mik-laj Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

mik-laj Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

VinayGb665 Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

mik-laj Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

VinayGb665 Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

mik-laj left a comment

Choose a reason for hiding this comment

Uh oh!

mik-laj commented Jun 25, 2020

Uh oh!

VinayGb665 commented Jun 26, 2020

Uh oh!

boring-cyborg bot commented Jun 26, 2020

Uh oh!

mik-laj commented Jun 26, 2020

Uh oh!

VinayGb665 commented Jun 26, 2020

Uh oh!

mik-laj commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VinayGb665 commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mik-laj commented Jun 26, 2020

Uh oh!

VinayGb665 commented Jun 26, 2020

Uh oh!

mik-laj commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VinayGb665 commented Jun 22, 2020 •

edited

Loading

mik-laj commented Jun 26, 2020 •

edited

Loading

VinayGb665 commented Jun 26, 2020 •

edited

Loading