You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Shawnsuun (via GitHub)" <gi...@apache.org> on 2026/10/07 01:12:54 UTC
[PR] Stop airflowctl dags update from unpausing Dags by default [airflow]
Shawnsuun opened a new pull request, #74383:
URL: https://github.com/apache/airflow/pull/74383
`airflowctl dags update <dag_id>` without `--is-paused` / `--no-is-paused` unpauses a paused Dag with no warning. Generated boolean flags default to `False`, so the CLI sends `{"is_paused": false, "scheduling_state": null}`, and the server accepts it as an unpause.
The same default makes `dags update --scheduling-state <state>` unusable. It always sends `is_paused: false` alongside the state, so the server's "exactly one of `is_paused` or `scheduling_state`" validator rejects it with a 422.
`CommandFactory` already has a list of datamodels whose generated bool flags keep the datamodel field default instead of `False` (`field_bool_default_datamodels`; `ClearTaskInstancesBody` is there so a bare `tasks clear` keeps `dry_run=True`). This PR adds `DAGPatchBody` to that list, so an omitted `--is-paused` stays `None`:
- A bare `dags update` is now rejected by the server (422, "Exactly one of `is_paused` or `scheduling_state` must be provided") and the Dag keeps its state.
- `dags update --scheduling-state <state>` works.
- `--is-paused` / `--no-is-paused`, and `dags pause` / `unpause` / `drain`, behave as before.
Alternative considered: a CLI-side check that refuses `dags update` without a flag. I didn't do that because it would repeat a rule the server already enforces and add special-casing to the generated-command path.
### Reproduction
I ran these against a local api-server built from `main` (SQLite, simple auth manager), with one Dag that starts paused. The `>>>` lines show the PATCH body airflowctl sent. Each case starts from a paused Dag.
Before:
```
airflowctl at main (3e302fd257)
$ airflowctl dags pause repro_dag
-> Dag state: is_paused=True scheduling_state=paused
$ airflowctl dags update repro_dag
>>> PATCH /api/v2/dags/repro_dag? body={"is_paused":false,"scheduling_state":null}
<<< 200
-> Dag state: is_paused=False scheduling_state=active
$ airflowctl dags update repro_dag --scheduling-state paused
>>> PATCH /api/v2/dags/repro_dag? body={"is_paused":false,"scheduling_state":"paused"}
<<< 422 Value error, Exactly one of `is_paused` or `scheduling_state` must be provided
-> Dag state: is_paused=True scheduling_state=paused
$ airflowctl dags update repro_dag --no-is-paused
>>> PATCH /api/v2/dags/repro_dag? body={"is_paused":false,"scheduling_state":null}
<<< 200
-> Dag state: is_paused=False scheduling_state=active
```
After:
```
airflowctl at main (3e302fd257) + this PR
$ airflowctl dags pause repro_dag
-> Dag state: is_paused=True scheduling_state=paused
$ airflowctl dags update repro_dag
>>> PATCH /api/v2/dags/repro_dag? body={"is_paused":null,"scheduling_state":null}
<<< 422 Value error, Exactly one of `is_paused` or `scheduling_state` must be provided
-> Dag state: is_paused=True scheduling_state=paused
$ airflowctl dags update repro_dag --scheduling-state paused
>>> PATCH /api/v2/dags/repro_dag? body={"is_paused":null,"scheduling_state":"paused"}
<<< 200
-> Dag state: is_paused=True scheduling_state=paused
$ airflowctl dags update repro_dag --no-is-paused
>>> PATCH /api/v2/dags/repro_dag? body={"is_paused":false,"scheduling_state":null}
<<< 200
-> Dag state: is_paused=False scheduling_state=active
```
I also checked an Airflow 3.3.2 api-server. There, `is_paused` is a required `bool`, so the `null` sent by a bare `dags update` is rejected with a 422 too. Separately, current `main` airflowctl already gets a 422 from 3.3.2 for every `dags update`/`pause`, because it sends `"scheduling_state": null`, which 3.3.2 rejects as an extra field. That issue exists without this PR and is left for a follow-up.
### Tests
- `uv run --project airflow-ctl pytest airflow-ctl/tests -q`: 426 passed. Both new test cases fail without the change.
- `prek run mypy-airflow-ctl --all-files`: passed
- `prek run --stage pre-commit --files <changed files>`: passed, except `generate-airflowctl-help-images`, which needs a working breeze locally. Help output is unchanged by this PR: the `dags` group help hash is identical with and without the change.
No newsfragment, because airflow-ctl doesn't use them.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: GitHub Copilot CLI (Claude Opus 5.5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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] Stop airflowctl updates from resetting omitted bool fields [airflow]
Posted by "Andrushika (via GitHub) via commits" <co...@airflow.apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Stop airflowctl updates from resetting omitted bool fields [airflow]
Posted by "potiuk (via GitHub) via commits" <co...@airflow.apache.org>.
potiuk merged PR #74383:
URL: https://github.com/apache/airflow/pull/74383
--
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] Stop airflowctl dags update from unpausing Dags by default [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #74383:
URL: https://github.com/apache/airflow/pull/74383#discussion_r4308139103
##########
airflow-ctl/src/airflowctl/ctl/cli_config.py:
##########
@@ -525,8 +525,9 @@ def __init__(self, file_path: str | Path | None = None):
]
# Datamodels whose generated bool flags follow the datamodel field defaults instead of
# defaulting to False, so the CLI keeps the API semantics (e.g. a bare ``tasks clear``
- # must keep ``dry_run=True`` and preview instead of clearing).
- self.field_bool_default_datamodels = ["ClearTaskInstancesBody"]
+ # must keep ``dry_run=True`` and preview instead of clearing, and a bare ``dags update``
+ # must not send ``is_paused=False`` and unpause the Dag).
+ self.field_bool_default_datamodels = ["ClearTaskInstancesBody", "DAGPatchBody"]
Review Comment:
`PoolPatchBody` has the same bug: `include_deferred: bool | None = None` gets a generated `--include-deferred` flag defaulting to `False`, so `airflowctl pools update --pool p --slots 10` silently sends `include_deferred=False` and flips it on a pool that had it enabled.
The allowlist fixes one datamodel at a time; the cause is `_get_bool_arg_default()` returning `False` for every bool field outside the list. Using the field's own default for every request-body datamodel (and leaving `None` out of the body) would fix the whole class and make the list unnecessary.
--
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] Stop airflowctl updates from resetting omitted bool fields [airflow]
Posted by "Shawnsuun (via GitHub)" <gi...@apache.org>.
Shawnsuun commented on code in PR #74383:
URL: https://github.com/apache/airflow/pull/74383#discussion_r4311084169
##########
airflow-ctl/src/airflowctl/ctl/cli_config.py:
##########
@@ -525,8 +525,9 @@ def __init__(self, file_path: str | Path | None = None):
]
# Datamodels whose generated bool flags follow the datamodel field defaults instead of
# defaulting to False, so the CLI keeps the API semantics (e.g. a bare ``tasks clear``
- # must keep ``dry_run=True`` and preview instead of clearing).
- self.field_bool_default_datamodels = ["ClearTaskInstancesBody"]
+ # must keep ``dry_run=True`` and preview instead of clearing, and a bare ``dags update``
+ # must not send ``is_paused=False`` and unpause the Dag).
+ self.field_bool_default_datamodels = ["ClearTaskInstancesBody", "DAGPatchBody"]
Review Comment:
Agreed, done in 2c83ba4340. `_get_bool_arg_default()` now uses the field's own default for every request-body datamodel, and the allowlist is gone. `pools update` without `--include-deferred` no longer sends `include_deferred=False`, with a test for it.
---
Drafted-by: GitHub Copilot (Claude Opus 5.5) (no human review before posting)
--
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] Stop airflowctl dags update from unpausing Dags by default [airflow]
Posted by "Shawnsuun (via GitHub)" <gi...@apache.org>.
Shawnsuun commented on code in PR #74383:
URL: https://github.com/apache/airflow/pull/74383#discussion_r4308626345
##########
airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py:
##########
@@ -1116,3 +1126,26 @@ def test_nested_single_key_list_is_not_split_into_rows(self, api_client_maker, c
rows = json.loads(capsys.readouterr().out)
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([], DAGPatchBody(), id="omitted-flag-does-not-unpause"),
+ pytest.param(
+ ["--scheduling-state", "paused"],
+ DAGPatchBody(scheduling_state=DagSchedulingState.PAUSED),
+ id="scheduling-state-alone",
+ ),
+ ],
+ )
Review Comment:
Good idea, added both explicit-flag cases so the test also pins that `--is-paused` / `--no-is-paused` still send the value. Thanks for the review!
---
Drafted-by: GitHub Copilot (Claude Opus 5.5) (no human review before posting)
--
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] Stop airflowctl dags update from unpausing Dags by default [airflow]
Posted by "SameerMesiah97 (via GitHub)" <gi...@apache.org>.
SameerMesiah97 commented on code in PR #74383:
URL: https://github.com/apache/airflow/pull/74383#discussion_r4307244155
##########
airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py:
##########
@@ -1116,3 +1126,26 @@ def test_nested_single_key_list_is_not_split_into_rows(self, api_client_maker, c
rows = json.loads(capsys.readouterr().out)
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([], DAGPatchBody(), id="omitted-flag-does-not-unpause"),
+ pytest.param(
+ ["--scheduling-state", "paused"],
+ DAGPatchBody(scheduling_state=DagSchedulingState.PAUSED),
+ id="scheduling-state-alone",
+ ),
+ ],
+ )
Review Comment:
You could add the following 2 test cases:
```
pytest.param(
["--is-paused"],
DAGPatchBody(is_paused=True),
id="explicit-pause",
),
pytest.param(
["--no-is-paused"],
DAGPatchBody(is_paused=False),
id="explicit-unpause",
),
```
--
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] Stop airflowctl updates from resetting omitted bool fields [airflow]
Posted by "Shawnsuun (via GitHub)" <gi...@apache.org>.
Shawnsuun commented on PR #74383:
URL: https://github.com/apache/airflow/pull/74383#issuecomment-6261576605
Thanks for the review. Addressed in 2c83ba4340:
- Generated bool flags now follow each field's default for every datamodel (see the inline thread), so this also fixes `pools update` and the other optional bool fields.
- `dags update` fails on the client with "Provide exactly one of --is-paused / --no-is-paused or --scheduling-state." when it gets neither or both, and it leaves `None` fields out of the request body.
- Tests that `--is-paused` / `--no-is-paused` reach the body as `True` / `False` were added in 4c2e3447b3, and there are matching cases for `--include-deferred` / `--no-include-deferred`.
- `airflow-ctl/RELEASE_NOTES.rst`: I didn't edit it, because its header says it is populated at release time and must not be edited in PRs. I described the user-visible change in the PR description instead. Happy to add a line here if you still want one.
---
Drafted-by: GitHub Copilot (Claude Opus 5.5) (no human review before posting)
--
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] Stop airflowctl updates from resetting omitted bool fields [airflow]
Posted by "Shawnsuun (via GitHub) via commits" <co...@airflow.apache.org>.
Shawnsuun commented on code in PR #74383:
URL: https://github.com/apache/airflow/pull/74383#discussion_r4431388930
##########
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:
Good catch. I added `backfill create` to the "User-visible change" section of the description: an omitted `--run-on-latest-version` is no longer sent as `False`, so the server now picks the value (the Dag's `rerun_with_latest_version`, then `[core] rerun_with_latest_version`, then `True` for backfills). A backfill that used to run on the original bundle version now runs on the latest one by default, and `--no-run-on-latest-version` keeps the old behavior.
##########
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:
Added `related: #73768` and `related: #73931` to the description, with a note that `pools update` without `--include-deferred` / `--no-include-deferred` now gets a 422 until the server change in #73931 lets a PATCH send only the changed fields.
##########
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:
Done in ae1f9d2c98. `test_dags_update_requires_exactly_one_state` now patches `AirflowConsole.print_as` like the test above. I checked it by disabling the guard: it now fails with `DID NOT RAISE SystemExit` instead of the tabulate `ValueError`.
--
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