potiuk commented on PR #68014: URL: https://github.com/apache/airflow/pull/68014#issuecomment-4627644160
Thanks for the airflowctl improvements, @ibobgunardi — picking these up together since the three PRs (this, #68000, #67912) overlap and there are a couple of concrete fixes needed before they land. **#68000 vs this PR — conflict.** These two edit the same two call sites in `cli_config.py` with opposite designs: #68000 makes required body fields `required=True` flags, while this one makes them **positional**. They're mutually exclusive and will textually conflict. The positional approach here is the right one — it matches the dev-list lazy consensus this file already follows for required primitive params. Could you fold #68000 into this PR (or close it) so we're not reviewing two contradictory designs for the same fields? **This PR — e2e test will break.** Once required scalar body fields become positional, the existing invocations in `airflow-ctl-tests/tests/airflowctl_tests/test_airflowctl_commands.py:66,72` still use the flag form and will fail with "unrecognized arguments": - `connections create --connection-id=test_con --conn-type=mysql …` → `connections create test_con mysql …` - `connections update --connection-id=test_con --conn-type=postgres` → `connections update test_con postgres` Worth a line in the PR body noting this is a breaking CLI change, so the release manager captures it under "Significant Changes". **#67912 — crash when `--limit` is omitted.** `airflowctl tasks states-for-dag-run <dag> <run>` without `--limit` passes `limit=None` into `execute_list` → `if limit <= 0:` → `TypeError: '<=' not supported between 'NoneType' and 'int'`. It's the first `execute_list` caller to expose `limit` to the CLI. Please default the flag (or coerce `None`→default before `super().execute_list`) and add a test that omits `--limit`. None of these are big — mostly coordination plus two small fixes. Happy to take another look once they're reconciled. --- Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
