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