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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
