Copilot commented on code in PR #63889:
URL: https://github.com/apache/airflow/pull/63889#discussion_r3066502972
##########
airflow-ctl/src/airflowctl/ctl/cli_config.py:
##########
@@ -645,10 +656,21 @@ def _get_func(args: Namespace, api_operation: dict,
api_client: Client = NEW_API
datamodel_param_name = parameter_key
if expanded_parameter in self.excluded_parameters:
continue
- if expanded_parameter in args_dict.keys():
+ if (
+ expanded_parameter in args_dict.keys()
+ and args_dict[expanded_parameter] is not None
+ ):
+ val = args_dict[expanded_parameter]
+ if isinstance(val, str) and expanded_parameter
in datamodel.model_fields:
+ import typing
+
+ annotation =
datamodel.model_fields[expanded_parameter].annotation
+ origin = typing.get_origin(annotation)
+ if origin is list or getattr(annotation,
"__origin__", None) is list:
+ val = [v.strip() for v in
val.split(",") if v.strip()]
Review Comment:
The comma-separated-to-list conversion logic won’t trigger for common
Pydantic annotations like `Optional[list[...]]` / `list[...] | None` (and often
`Annotated[...]`). For example, `ClearTaskInstancesBody.task_ids` is `list[...]
| None` in `api/datamodels/generated.py`, so `typing.get_origin(annotation)`
will be a union, not `list`, and the value will stay a string (causing
`model_validate` to fail). Consider unwrapping `Annotated`, then handling
unions by checking `typing.get_args()` for a `list` origin before splitting.
Also, `import typing` can be moved to module scope to avoid repeated imports
inside the arg loop.
##########
airflow-ctl/src/airflowctl/ctl/cli_config.py:
##########
@@ -645,10 +656,21 @@ def _get_func(args: Namespace, api_operation: dict,
api_client: Client = NEW_API
datamodel_param_name = parameter_key
if expanded_parameter in self.excluded_parameters:
continue
- if expanded_parameter in args_dict.keys():
+ if (
+ expanded_parameter in args_dict.keys()
+ and args_dict[expanded_parameter] is not None
+ ):
+ val = args_dict[expanded_parameter]
+ if isinstance(val, str) and expanded_parameter
in datamodel.model_fields:
+ import typing
+
+ annotation =
datamodel.model_fields[expanded_parameter].annotation
+ origin = typing.get_origin(annotation)
+ if origin is list or getattr(annotation,
"__origin__", None) is list:
+ val = [v.strip() for v in
val.split(",") if v.strip()]
method_params[parameter_key][
self._sanitize_method_param_key(expanded_parameter)
- ] = args_dict[expanded_parameter]
+ ] = val
Review Comment:
New behavior is introduced here (auto-splitting comma-separated CLI inputs
into `list[...]` fields) but there are no unit tests covering it. Since this
impacts request-body validation (and new `taskinstance clear` options), please
add a CLI-config test that proves e.g. `--task-ids t1,t2` becomes `['t1','t2']`
for `ClearTaskInstancesBody.task_ids`, and that non-list string fields are
unaffected.
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -434,6 +435,12 @@ def xcom(self):
"""Operations related to XComs."""
return XComOperations(self)
+ @lru_cache() # type: ignore[prop-decorator]
+ @property
+ def tasks(self):
Review Comment:
The new `Client` property is named `tasks`, but it actually exposes task
*instance* operations (and the PR description says `task_instances`). `tasks`
is ambiguous (could mean DAG tasks, not instances) and doesn’t match the naming
style of other resource properties (`dag_runs`, `backfills`, etc.). Consider
renaming this property to something explicit like `task_instances` (or
`taskinstance` to mirror the CLI group), and update call sites/tests
accordingly.
```suggestion
def task_instances(self):
```
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -434,6 +435,12 @@ def xcom(self):
"""Operations related to XComs."""
return XComOperations(self)
+ @lru_cache() # type: ignore[prop-decorator]
+ @property
+ def tasks(self):
+ """Operations related to task instances."""
+ return TaskInstanceOperations(self)
Review Comment:
PR description says a `task_instances` property was added to `Client`, but
the implementation adds `tasks`. Please either update the PR description or
align the code to avoid confusion for reviewers and API consumers.
--
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]
