You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "evgenii-prusov (via GitHub)" <gi...@apache.org> on 2025/07/24 18:38:02 UTC
[PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
evgenii-prusov opened a new pull request, #53726:
URL: https://github.com/apache/airflow/pull/53726
# Feature Proposal: Add YAML Support to Airflow Variables Import Command
## Problem Statement
Apache Airflow currently has an inconsistency in its CLI commands for importing variables and connections:
- **`airflow connections import`**: Supports multiple file formats (JSON, YAML, ENV) using a flexible secret parser
- **`airflow variables import`**: Only supports JSON format with hardcoded `json.load()` and `json.dump()`
This inconsistency damages user experience and limits flexibility when managing Airflow variables, especially for teams that prefer YAML for configuration management.
This change brings the `airflow variables import` command to feature parity with `airflow connections import` by adding support for YAML and ENV file formats.
Key changes:
- Replace hardcoded `json.load()` with flexible `load_variables()` function
- Add format detection based on file extension (.json, .yaml, .yml, .env)
- Handle ENV files differently from JSON/YAML (single values vs. preserving data types)
- Add comprehensive tests for all supported formats
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W merged PR #53726:
URL: https://github.com/apache/airflow/pull/53726
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2248453979
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I guess that one still needs to be resolved?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2254429245
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I think it's pretty common pattern in CLI tools. I personally do not like to have ugly (nor even nice) stack trace as result of unhandled exception so try/except Exception allows to have a nice one line exception message - ideally in 🟥 ....
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on PR #53726:
URL: https://github.com/apache/airflow/pull/53726#issuecomment-3114490078
Hi @Lee-W , Following up on our quick chat in Prague last Saturday - would love to get some eyes on this when someone has a moment. Thanks! 🙏
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2252946775
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
Got it. I just took a deeper look. I just noticed all the cases you mentioned are handled above. Do we still need this `except Exception as e:` block?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2253136857
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
As we work with user files, I think it's still useful for catching these exceptions:
- PermissionError (file access issues)
- OSError (other I/O errors beyond FileNotFoundError)
- MemoryError (for very large files)
- UnicodeDecodeError (encoding issues)
I can see that pattern in connection_command too.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2235838491
##########
airflow-core/src/airflow/secrets/local_filesystem.py:
##########
@@ -233,10 +233,23 @@ def load_variables(file_path: str) -> dict[str, str]:
log.debug("Loading variables from a text file")
secrets = _parse_secret_file(file_path)
- invalid_keys = [key for key, values in secrets.items() if isinstance(values, list) and len(values) != 1]
- if invalid_keys:
- raise AirflowException(f'The "{file_path}" file contains multiple values for keys: {invalid_keys}')
- variables = {key: values[0] if isinstance(values, list) else values for key, values in secrets.items()}
+
+ # Determine a file format to handle lists correctly
+ ext = file_path.rsplit(".", 2)[-1].lower()
Review Comment:
Same
##########
airflow-core/src/airflow/secrets/local_filesystem.py:
##########
@@ -233,10 +233,23 @@ def load_variables(file_path: str) -> dict[str, str]:
log.debug("Loading variables from a text file")
secrets = _parse_secret_file(file_path)
- invalid_keys = [key for key, values in secrets.items() if isinstance(values, list) and len(values) != 1]
- if invalid_keys:
- raise AirflowException(f'The "{file_path}" file contains multiple values for keys: {invalid_keys}')
- variables = {key: values[0] if isinstance(values, list) else values for key, values in secrets.items()}
+
+ # Determine a file format to handle lists correctly
+ ext = file_path.rsplit(".", 2)[-1].lower()
+ is_env_file = ext == "env"
+
+ variables = {}
+ for key, secret_values in secrets.items():
+ if is_env_file and isinstance(secret_values, list):
+ # Only ENV files: Check for duplicate keys
+ if len(secret_values) > 1:
+ raise AirflowException(
Review Comment:
Let's create a custom exception for it instead of using `AirflowException`
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
Is it possible to catch a narrower exception?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #53726:
URL: https://github.com/apache/airflow/pull/53726#issuecomment-3159123772
LGTM. Thanks for the help! Let's merge 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on PR #53726:
URL: https://github.com/apache/airflow/pull/53726#issuecomment-3134357643
Hi @Lee-W , @shahar1 , I've fixed all suggestions.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2250031232
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I've reimplemented the proposed change. However, during development I discovered an important limitation:
The current load_variables() implementation in LocalFilesystemBackend doesn't support the description property for variables. This means that moving the CLI import to use this shared logic removes the existing ability to import variables with descriptions from **JSON files**.
**Decision Needed:**
We need to decide the priority between:
1. New capability: YAML and ENV file support for variable imports
2. Existing capability: JSON files with variable descriptions
**Options:**
- Accept the breaking change and document it
- Revert to custom JSON parsing in CLI while keeping error handling improvements
- Enhance LocalFilesystemBackend.load_variables() to support descriptions
Please advise on the preferred approach. The current implementation is ready but introduces this breaking change.
P.S. @Lee-W, parsing level exceptions `yaml.MarkedYAMLError`, `JSONDecodeError` handled in parser functions `_parse_yaml_file` and `_parse_json_file` respectively and transformed into `AirflowFileParseException` later. As for FileNotFound and other OSErrors they are handled directly in `variables_import` function.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2251816721
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
> Yep, but fail early is still better
I'm not sure I've got you point. Could you please clarify what exactly do you mean?
Current error handling strategy (as I understand it) is to catch parsing errors from 3 low-level parser functions `_parse_<env|json|yaml>_file`, repackage and return them as lists of `FileSyntaxError` to the caller `_parse_secret_file` and let the caller raise the `AirflowFileParseException` with all collected errors, which let the users see all the issues in their configuration files at once rather than making them fix one by 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2255697601
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,11 +86,20 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ var_json = load_variables(args.file)
+ except AirflowUnsupportedFileTypeException as e:
+ raise SystemExit(str(e))
+ except AirflowFileParseException as e:
+ raise SystemExit(str(e))
+ except VariableNotUnique as e:
+ raise SystemExit(str(e))
Review Comment:
Then I guess this is the last one we need to handle.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #53726:
URL: https://github.com/apache/airflow/pull/53726#issuecomment-3126341275
I'll take a look today. Just come back from my vacation
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2249091313
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
yep, I think this is still missed
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2250031232
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I've implemented the requested alignment between airflow variables import and `LocalFilesystemBackend` using the centralized `load_variables()` function. However, during development I discovered an important limitation:
**Breaking Change Identified:**
The current load_variables() implementation in LocalFilesystemBackend doesn't support the description property for variables. This means that moving the CLI import to use this shared logic removes the existing ability to import variables with descriptions from **JSON files**.
**Current State:**
- ✅ CLI now uses centralized error handling and supports YAML/ENV formats
- ✅ Better error messages and file format validation
- ❌ Lost capability: JSON files with variable descriptions no longer work
- ❌ No test coverage: I couldn't find tests validating description imports
**Decision Needed:**
We need to decide the priority between:
1. New capability: YAML and ENV file support for variable imports
2. Existing capability: JSON files with variable descriptions
**Options:**
- Accept the breaking change and document it
- Enhance LocalFilesystemBackend.load_variables() to support descriptions
- Revert to custom JSON parsing in CLI while keeping error handling improvements
Please advise on the preferred approach. The current implementation is ready but introduces this breaking change.
P.S. @Lee-W, parsing level exceptions `yaml.MarkedYAMLError`, `JSONDecodeError` handled in parser functions `_parse_yaml_file` and `_parse_json_file` respectively and transformed into `AirflowFileParseException` later. As for FileNotFound and other OSErrors they are handled directly in `variables_import` function.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2251200424
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
> Enhance LocalFilesystemBackend.load_variables() to support descriptions
ideally this
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
> parsing level exceptions yaml.MarkedYAMLError, JSONDecodeError handled in parser functions _parse_yaml_file and _parse_json_file respectively and transformed into AirflowFileParseException later. As for FileNotFound and other OSErrors they are handled directly in variables_import function.
Yep, but fail early is still better
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
> Happy to tackle the description functionality improvements if you're interested and can point me in the right direction.
Not that familiar with this piece, but I would try to see how I can help when I have more bandwidth. Thanks!
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,11 +86,20 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ var_json = load_variables(args.file)
+ except AirflowUnsupportedFileTypeException as e:
+ raise SystemExit(str(e))
+ except AirflowFileParseException as e:
+ raise SystemExit(str(e))
+ except VariableNotUnique as e:
+ raise SystemExit(str(e))
Review Comment:
```suggestion
except (AirflowUnsupportedFileTypeException, AirflowFileParseException, VariableNotUnique) as e:
raise SystemExit(str(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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2232797683
##########
airflow-core/src/airflow/secrets/local_filesystem.py:
##########
@@ -233,10 +233,23 @@ def load_variables(file_path: str) -> dict[str, str]:
log.debug("Loading variables from a text file")
secrets = _parse_secret_file(file_path)
- invalid_keys = [key for key, values in secrets.items() if isinstance(values, list) and len(values) != 1]
- if invalid_keys:
- raise AirflowException(f'The "{file_path}" file contains multiple values for keys: {invalid_keys}')
- variables = {key: values[0] if isinstance(values, list) else values for key, values in secrets.items()}
+
+ # Determine a file format to handle lists correctly
+ ext = file_path.rsplit(".", 2)[-1].lower()
Review Comment:
I'd rather use `Pathlib` method to determine file's suffix rather than string manipulation
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2238384517
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I think this is not resolved. What I meant is whether we could change `except Exception` to something like `except JSONDecodeError` but contains all the possible exceptions expected
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2250034579
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I also found this old https://github.com/apache/airflow/issues/37532 which shows that working with variable descriptions had limitations even before this change. The issue highlighted that variable exports from the UI didn't include description fields, requiring manual updates when importing to other environments. This suggests the description functionality was already incomplete in the broader ecosystem.
Happy to tackle the description functionality improvements if you're interested and can point me in the right direction.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "evgenii-prusov (via GitHub)" <gi...@apache.org>.
evgenii-prusov commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2250031232
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I've reimplemented the proposed change. However, during development I discovered an important limitation:
**Breaking Change Identified:**
The current load_variables() implementation in LocalFilesystemBackend doesn't support the description property for variables. This means that moving the CLI import to use this shared logic removes the existing ability to import variables with descriptions from **JSON files**.
**Decision Needed:**
We need to decide the priority between:
1. New capability: YAML and ENV file support for variable imports
2. Existing capability: JSON files with variable descriptions
**Options:**
- Accept the breaking change and document it
- Revert to custom JSON parsing in CLI while keeping error handling improvements
- Enhance LocalFilesystemBackend.load_variables() to support descriptions
Please advise on the preferred approach. The current implementation is ready but introduces this breaking change.
P.S. @Lee-W, parsing level exceptions `yaml.MarkedYAMLError`, `JSONDecodeError` handled in parser functions `_parse_yaml_file` and `_parse_json_file` respectively and transformed into `AirflowFileParseException` later. As for FileNotFound and other OSErrors they are handled directly in `variables_import` function.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #53726:
URL: https://github.com/apache/airflow/pull/53726#issuecomment-3138346860
It would be great if you could let me know what you think about the `Exception` thing :)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2254429245
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I think it's pretty common pattern in CLI tools. I personally do not like to have ugly (nor even nice) stack trace as result of unhandled exception in CLI so try/except Exception allows to have a nice one line exception message - ideally in 🟥 ....
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): add YAML/ENV support to `airflow variables import` [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #53726:
URL: https://github.com/apache/airflow/pull/53726#discussion_r2253295785
##########
airflow-core/src/airflow/cli/commands/variable_command.py:
##########
@@ -81,20 +81,25 @@ def variables_import(args, session):
"""Import variables from a given file."""
if not os.path.exists(args.file):
raise SystemExit("Missing variables file.")
- with open(args.file) as varfile:
- try:
- var_json = json.load(varfile)
- except JSONDecodeError:
- raise SystemExit("Invalid variables file.")
+
+ try:
+ # Use load_variables which supports JSON, YAML, and ENV formats
+ var_dict = load_variables(args.file)
+ except Exception as e:
Review Comment:
I still like catching explicit exceptions more, but if this pattern is used elsewhere. I'm also ok with 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org