add convert_env_vars_from_list_of_dicts on backwards_compat_converters by s21lee · Pull Request #47050 · apache/airflow

s21lee · 2025-02-25T09:55:29Z

This modification was made with backward compatibility.
In the previous version, env_vars was passed as a list of dictionaries. However, in the current version, if it is not passed as a V1EnvVar type, it causes issues in other hooks and other components that use these variables.

To avoid this, I added the necessary code to convert the string dictionary list format into the V1EnvVar type.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Signed-off-by: s21.lee <s21.lee@samsung.com>

boring-cyborg · 2025-02-25T09:55:36Z

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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy 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.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
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://s.apache.org/airflow-slack

potiuk · 2025-02-25T13:05:23Z

I don't think we ever look at PRs that do not explain what they are for in the description. I think it's quite an important thing to explain your intent if you want reviewers to take a look at it @s21lee

s21lee · 2025-02-26T04:29:08Z

I don't think we ever look at PRs that do not explain what they are for in the description. I think it's quite an important thing to explain your intent if you want reviewers to take a look at it @s21lee

Sorry for the lack of explanation—I had to create the PR in a hurry.

This modification was made with backward compatibility in mind.
In the previous version, env_vars was passed as a list of dictionaries. However, in the current version, if it is not passed as a V1EnvVar type, it causes issues in hooks and other components that use these variables.

To avoid this, I added the necessary code to convert the string dictionary list format into the V1EnvVar type.

Signed-off-by: s21.lee <s21.lee@samsung.com>

...rs/cncf/kubernetes/tests/unit/cncf/kubernetes/backcompat/test_backwards_compat_converters.py

…t/test_backwards_compat_converters.py

s21lee · 2025-03-10T01:04:15Z

Hi, @jedcunningham @hussein-awala
If you have a time, would you like to review it?
Thank you.

s21lee · 2025-04-01T10:57:42Z

Hi, @jedcunningham @hussein-awala
I'm still waiting :)

github-actions · 2025-07-29T00:21:13Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

Nataneljpwd · 2026-03-26T06:55:20Z

...f/kubernetes/src/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

+    if isinstance(env_vars, list) and \
+        all(isinstance(item, dict) and "name" in item and "value" in item for item in env_vars):
+        return [k8s.V1EnvVar(name=env_var.get("name"), value=env_var.get("value")) for env_var in env_vars]
+    return env_vars


So if it is a list of dicts, yet one does not have the name / value key (or there is a none value) this will just return the env_vars which will raise an error, I think this needs to be addressed as well, also what about just allowing to pass a dict where the key is the env name, and the value is the value?

And also I suggest adding tests for this

jscheffl · 2026-04-03T08:54:18Z

Backwards-compatibility is a bummer. Can you point me to the docs or specs of an old version where this list[dict[str, str]] is being defined? I could not find any.

And if it a very old version I'd propose to add a deprecation marker and plan to clean such behavior up for one next release.

Signed-off-by: s21.lee <s21.lee@samsung.com>

jscheffl · 2026-04-03T09:24:46Z

...f/kubernetes/src/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

+            stacklevel=2,
+        )
+        return [_env_var_dict_to_v1(d, i) for i, d in enumerate(env_vars)]
+    raise AirflowException(


We defined as a community that that generic AirflowException is only existing for historic reasons. Codebase is in cleaning. Can you please use either a standard Python Exception (ValueErrormight be fitting here) or define a specific execption for this problem?

Thank you for your code review. I will update it.

jscheffl · 2026-04-03T09:26:31Z

...f/kubernetes/src/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

+    all_v1 = all(isinstance(x, k8s.V1EnvVar) for x in env_vars)
+    all_dict = all(isinstance(x, dict) for x in env_vars)
+    if all_v1:
+        return env_vars
+    if all_dict:


Both variables are declared only for a single use. I'd assume it can be evaluated inline directly

Suggested change

all_v1 = all(isinstance(x, k8s.V1EnvVar) for x in env_vars)

all_dict = all(isinstance(x, dict) for x in env_vars)

if all_v1:

return env_vars

if all_dict:

if all(isinstance(x, k8s.V1EnvVar) for x in env_vars):

return env_vars

if all(isinstance(x, dict) for x in env_vars):

Thank you for comments. I will add

Signed-off-by: s21.lee <s21.lee@samsung.com>

s21lee · 2026-04-03T10:15:48Z

Backwards-compatibility is a bummer. Can you point me to the docs or specs of an old version where this list[dict[str, str]] is being defined? I could not find any.

And if it a very old version I'd propose to add a deprecation marker and plan to clean such behavior up for one next release.

I couldn’t find an official Airflow doc or type spec that defines env_vars as list[{"name": ..., "value": ...}] either. In practice it shows up from templated YAML/JSON and older community examples, not from a first-class API description. I’ve added a note in the convert_env_vars docstring stating that this list-of-dicts shape was not documented as a stable public API and that the supported styles are dict[str, str] (name → value) and list[k8s.V1EnvVar].

jscheffl · 2026-04-03T16:30:31Z

...f/kubernetes/src/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

+    if isinstance(name, str) and name and "value" in item:
+        return k8s.V1EnvVar(name=name, value=item["value"])
+    if "name" in item and not isinstance(item["name"], str):
+        raise AirflowException(


There are some more AirflowExceptionto be replaced with ValueError.

jscheffl · 2026-04-03T16:31:12Z

...f/kubernetes/src/airflow/providers/cncf/kubernetes/backcompat/backwards_compat_converters.py

-
-
-def convert_env_vars_or_raise_error(env_vars: list[k8s.V1EnvVar] | dict[str, str]) -> list[k8s.V1EnvVar]:
+    if not isinstance(env_vars, list):


What would be the type that is "working" and not breaking functionality that is not a list? (And that is passed w/o warning as accepted)

add convert_env_vars_from_list_of_dicts on backwards_compat_converters

2d07801

Signed-off-by: s21.lee <s21.lee@samsung.com>

s21lee requested review from hussein-awala and jedcunningham as code owners February 25, 2025 09:55

boring-cyborg bot added the provider:cncf-kubernetes Kubernetes (k8s) provider related issues label Feb 25, 2025

s21lee and others added 2 commits March 7, 2025 15:51

Merge branch 'apache:main' into add-convert_env_vars_from_list_of_dicts

6e58d33

fix unit test error

e0c473e

Signed-off-by: s21.lee <s21.lee@samsung.com>

s21lee commented Mar 7, 2025

View reviewed changes

...rs/cncf/kubernetes/tests/unit/cncf/kubernetes/backcompat/test_backwards_compat_converters.py Outdated Show resolved Hide resolved

s21lee added 2 commits March 7, 2025 17:33

Update providers/cncf/kubernetes/tests/unit/cncf/kubernetes/backcompa…

fa41e61

…t/test_backwards_compat_converters.py

Merge branch 'apache:main' into add-convert_env_vars_from_list_of_dicts

76a617e

s21lee added 2 commits April 12, 2025 10:05

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

b5a53b4

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

842fdff

s21lee marked this pull request as draft May 23, 2025 00:13

s21lee marked this pull request as ready for review May 23, 2025 00:13

s21lee added 3 commits May 23, 2025 09:13

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

1850e69

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

cb658d1

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

fb44504

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 29, 2025

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

5b4fc07

github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 1, 2025

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

7a3e491

Nataneljpwd reviewed Mar 26, 2026

View reviewed changes

Merge branch 'main' into add-convert_env_vars_from_list_of_dicts

418ea6b

s21lee requested a review from jscheffl as a code owner April 2, 2026 11:26

Merge branch 'apache:main' into add-convert_env_vars_from_list_of_dicts

3ae47ca

update method by review

1d68540

Signed-off-by: s21.lee <s21.lee@samsung.com>

jscheffl reviewed Apr 3, 2026

View reviewed changes

apply review comments

0910e07

Signed-off-by: s21.lee <s21.lee@samsung.com>

jscheffl reviewed Apr 3, 2026

View reviewed changes



		def convert_env_vars_or_raise_error(env_vars: list[k8s.V1EnvVar] \| dict[str, str]) -> list[k8s.V1EnvVar]:
		if not isinstance(env_vars, list):

Conversation

s21lee commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented Feb 25, 2025

Uh oh!

potiuk commented Feb 25, 2025

Uh oh!

s21lee commented Feb 26, 2025

Uh oh!

Uh oh!

s21lee commented Mar 10, 2025

Uh oh!

s21lee commented Apr 1, 2025

Uh oh!

github-actions bot commented Jul 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jscheffl commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s21lee commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

s21lee commented Feb 25, 2025 •

edited

Loading