1fanwang commented on PR #70347:
URL: https://github.com/apache/airflow/pull/70347#issuecomment-5195637920
> Thanks — the core fix is correct and worth having. `args = {command,
powershell, cmdlet}` built a **set**, so equal values collapsed into one entry,
and `exactly_one` then counted truthiness rather than presence — meaning an
empty-string `command` read as "not provided". Switching to explicit `is not
None` checks fixes both problems, and carrying the same correction into
`execute()` keeps it consistent.
>
> One blocking concern: this PR also **removes `kwargs.setdefault("task_id",
cmdlet)`**, so a Dag that relied on the cmdlet supplying the task_id now gets a
different task_id. That changes task identity — history, logs, XComs and UI
links all key off it — so it isn't a refactor, it's a breaking change for those
users.
>
> Please either:
>
> 1. split the `task_id` removal into its own PR so the (uncontroversial)
validation fix can land immediately, or
> 2. keep it here but add a newsfragment in `airflow-core/newsfragments/`
explaining that PsrpOperator no longer defaults `task_id` to `cmdlet` and what
users must do — plus a line in the provider changelog.
>
> For context: #70656 makes the same validation fix without the `task_id`
change. It was opened six days after this one, so I'm closing it in favour of
this PR — which means the validation fix now depends on this one moving. Option
1 would unblock it fastest.
>
> These are judgement calls rather than mechanical fixes — I'd welcome your
own reasoning in reply. My review was AI-assisted and shouldn't be treated as
settled.
>
> Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
thanks! went with option 1, so this PR is validation-only now.
--
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]
