Andrushika commented on code in PR #74383:
URL: https://github.com/apache/airflow/pull/74383#discussion_r4431018750
##########
airflow-ctl/src/airflowctl/ctl/cli_config.py:
##########
@@ -779,11 +775,16 @@ def _find_subgroup_suffix(self, operation_group_name:
str, operation_name: str)
return suffix
return None
- def _get_bool_arg_default(self, parameter_type: str, field_default: Any)
-> bool | None:
- """Get default for a generated bool flag: the datamodel field default
for datamodels in ``field_bool_default_datamodels``, otherwise False."""
- if parameter_type in self.field_bool_default_datamodels and (
- field_default is None or isinstance(field_default, bool)
- ):
+ @staticmethod
+ def _get_bool_arg_default(field_default: Any) -> bool | None:
+ """
+ Get the default for a generated bool flag from its datamodel field
default.
+
+ An omitted flag must keep the API semantics: a bare ``tasks clear``
keeps ``dry_run=True``,
+ and an optional field such as ``is_paused`` or ``include_deferred``
stays ``None`` so it is
+ not sent at all instead of overwriting the stored value with ``False``.
+ """
+ if field_default is None or isinstance(field_default, bool):
return field_default
Review Comment:
We might need to list `backfill create` under "User-visible change" in the
description too. An omitted `--run-on-latest-version` now lets the server
choose, so a backfill runs on the latest bundle version by default instead of
the original one.
##########
airflow-ctl-tests/tests/airflowctl_tests/test_airflowctl_commands.py:
##########
@@ -138,7 +138,7 @@ def date_param():
"pools list",
"pools get test_pool",
"pools get test_pool -o yaml",
- "pools update --pool=test_pool --slots=10",
+ "pools update --pool=test_pool --slots=10 --no-include-deferred",
Review Comment:
We might need to add `related: #73768` and `related: #73931` to the
description here. Without the flag, `pools update` now gets a 422 from the
server, and #73931 is the server change that would let the client send only the
fields it changes.
##########
airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py:
##########
@@ -1427,6 +1438,105 @@ def
test_nested_single_key_list_is_not_split_into_rows(self, api_client_maker, c
assert [row["dag_run_id"] for row in rows] == ["manual_run"]
assert rows[0]["conf"] == ["a", "b"]
+ @pytest.mark.parametrize(
+ ("extra_argv", "expected_body"),
+ [
+ pytest.param(
+ ["--scheduling-state", "paused"],
+ DAGPatchBody(scheduling_state=DagSchedulingState.PAUSED),
+ id="scheduling-state-alone",
+ ),
+ pytest.param(["--is-paused"], DAGPatchBody(is_paused=True),
id="explicit-pause"),
+ pytest.param(["--no-is-paused"], DAGPatchBody(is_paused=False),
id="explicit-unpause"),
+ ],
+ )
+ @mock.patch.object(AirflowConsole, "print_as", autospec=True)
+ @mock.patch.object(DagsOperations, "update", autospec=True)
+ def test_dags_update_sends_only_given_state(
+ self, mocked_update, mocked_print_as, extra_argv, expected_body
+ ):
+ args = cli_parser.get_parser().parse_args(["dags", "update", "my_dag",
*extra_argv])
+
+ args.func(args, api_client=mock.MagicMock(spec=Client))
+
+ assert mocked_update.call_args.kwargs["dag_body"] == expected_body
+
+ @pytest.mark.parametrize(
+ "extra_argv",
+ [
+ pytest.param([], id="no-state-does-not-unpause"),
+ pytest.param(["--is-paused", "--scheduling-state", "paused"],
id="both-states"),
+ ],
+ )
+ @mock.patch.object(DagsOperations, "update", autospec=True)
Review Comment:
Nit: we might need to patch `AirflowConsole.print_as` here too, like the
test above. Without the check, this test fails on a tabulate `ValueError` and
not on `assert_not_called()`.
--
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]
