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/12/11 17:57:07 UTC
[GitHub] [airflow] SevakAvet opened a new pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
SevakAvet opened a new pull request #13022:
URL: https://github.com/apache/airflow/pull/13022
Add support of YAML serialisation/deserialisation in Variables alongside with JSON, respective parameters (serialize_yaml , deserialize_yaml) are added to get/set/setdefault functions.
----------------------------------------------------------------
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] SevakAvet commented on pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
SevakAvet commented on pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#issuecomment-743448201
> If (and it's still an if!) we add this, you should also support using it in variables so `{{ var.yaml.my_var }}` works too.
Sure, I'll add that once it's clear this feature is needed
----------------------------------------------------------------
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] ashb commented on a change in pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#discussion_r541323083
##########
File path: airflow/models/variable.py
##########
@@ -129,22 +142,37 @@ def get(
else:
if deserialize_json:
return json.loads(var_val)
+ elif deserialize_yaml:
+ return yaml.load(var_val)
Review comment:
```suggestion
return yaml.safe_load(var_val)
```
----------------------------------------------------------------
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] ashb commented on pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#issuecomment-743445719
@jtvmatos Care to weigh in here? Convince me that YAML is worth it?
Also does yaml need to be supported on decoding too, or is just accepting YAML but converting and storing it as JSON 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] SevakAvet commented on pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
SevakAvet commented on pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#issuecomment-743444538
> Can you explain why we need this, and why json isn't enough?
>
> Mostly I'm not sure I like the the precedent of supporting multiple encoding types for variables.
That's a good question and maybe it should be addressed to original issue author, as far as I understood some people might want to store yamls in variables instead of json because it's more readable (?). Anyways, I just wanted to work on some open issue and contribute :)
----------------------------------------------------------------
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] SevakAvet commented on a change in pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
SevakAvet commented on a change in pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#discussion_r541335626
##########
File path: airflow/models/variable.py
##########
@@ -82,7 +83,7 @@ def val(cls): # pylint: disable=no-self-argument
return synonym('_val', descriptor=property(cls.get_val, cls.set_val))
@classmethod
- def setdefault(cls, key, default, deserialize_json=False):
+ def setdefault(cls, key, default, deserialize_json=False, deserialize_yaml=False):
Review comment:
I was thinking about that, if we want to support more than one format, would that make sense to have a parameter like serialize /deserialize of type str, which inputs could be one of json, yaml, etc.? Don't like this because it's not backward compatible. Or this could have been another argument and original would eventually be removed.
I can attach some screenshots of UI tomorrow.
----------------------------------------------------------------
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] ashb commented on pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#issuecomment-743441863
Can you explain why we need this, and why json isn't enough?
Mostly I'm not sure I like the the precedent of supporting multiple encoding types for variables.
----------------------------------------------------------------
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] ashb commented on pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#issuecomment-743446333
See also https://github.com/apache/airflow/issues/13014 -- how should we handle YAML values there?
(That's why I'm maybe asking if we could just accept YAML input but convert to JSON for storage.)
----------------------------------------------------------------
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] ashb commented on a change in pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13022:
URL: https://github.com/apache/airflow/pull/13022#discussion_r541327051
##########
File path: airflow/models/variable.py
##########
@@ -82,7 +83,7 @@ def val(cls): # pylint: disable=no-self-argument
return synonym('_val', descriptor=property(cls.get_val, cls.set_val))
@classmethod
- def setdefault(cls, key, default, deserialize_json=False):
+ def setdefault(cls, key, default, deserialize_json=False, deserialize_yaml=False):
Review comment:
I wonder if we could do something better than these two vars.
Hmmmm.
How does UI display one of these YAML serialized variables?
----------------------------------------------------------------
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] SevakAvet closed pull request #13022: Fix #12480 Add support of YAML serialisation/deserialisation in Variables
Posted by GitBox <gi...@apache.org>.
SevakAvet closed pull request #13022:
URL: https://github.com/apache/airflow/pull/13022
----------------------------------------------------------------
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