Fix PsrpOperator option checks - #70347
Conversation
command, powershell, cmdlet, arguments and parameters are template fields, rendered after __init__ runs. The constructor validated their combination and derived task_id from cmdlet, all reading the un-rendered Jinja expressions. Move the validation into execute(), which runs after rendering. This drops the cmdlet-derived task_id default (a construction-time read of a template field that cannot move): task_id must now be passed explicitly when using cmdlet. related: apache#70296 Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com> # Conflicts: # scripts/ci/prek/validate_operators_init_exemptions.txt
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
command is a template field, so validating it in __init__ checked the raw Jinja expression, not the rendered value. Add a test that renders a command resolving to an empty string, then executes: it fails on the pre-fix source (the empty command slips past __init__ and reaches execution) and passes with validation in execute(). Signed-off-by: 1fanwang <1fannnw@gmail.com>
exactly_one deduped equal values before counting them, so passing the same value to two of command/powershell/cmdlet passed validation. Pass the options positionally so each is counted. Drop the manual changelog note; the provider release manager regenerates the changelog from git log. Signed-off-by: 1fanwang <1fannnw@gmail.com>
The mutual-exclusivity and arguments/parameters checks are about which options the Dag author provided, not what the templates render to. Keying them off rendered truthiness dropped a provided option that rendered to a falsy value, and let a second option slip through when the first rendered empty. Check is-not-None (usage) instead, and dispatch the chosen option consistently. Signed-off-by: 1fanwang <1fannnw@gmail.com>
Only the constructor can tell whether an option was passed: with render_template_as_native_obj a provided field can render to None, so the same check in execute() reports a supplied argument as missing. Compare against None rather than truthiness, because a provided option can itself be empty. related: apache#70505 Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
potiuk
left a comment
There was a problem hiding this comment.
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:
- split the
task_idremoval into its own PR so the (uncontroversial) validation fix can land immediately, or - keep it here but add a newsfragment in
airflow-core/newsfragments/explaining that PsrpOperator no longer defaultstask_idtocmdletand 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
| :param cmdlet: | ||
| cmdlet to execute on remote host (templated). Also used as the default | ||
| value for `task_id`. | ||
| cmdlet to execute on remote host (templated). |
There was a problem hiding this comment.
This docstring edit is the visible half of the behaviour change — dropping "Also used as the default value for task_id" alongside removing the setdefault.
Documenting it here is right, but a docstring is not where users find out that their task_ids changed. That needs the newsfragment/changelog entry described in the review body.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
There was a problem hiding this comment.
Reverted with the setdefault , so the docstring documents the task_id default again
Dropping it changes task identity for Dags that rely on it, so history, logs, XComs and UI links all move. That is a breaking change and deserves its own review rather than riding along with a validation fix. Splitting it out lets the option checks land on their own and unblocks the burn-down in apache#70296, since apache#70656 was closed in favour of this PR. The removal follows as a separate change with a provider changelog note. Signed-off-by: 1fanwang <1fannnw@gmail.com>
thanks! went with option 1, so this PR is validation-only now. |
command,powershellandcmdletare mutually exclusive, andarguments/parametersare only valid alongside the latter two. All five are template fields, and every check was written as a truthiness test:exactly_onewas handed a set literal, so passing the same value to two options deduped to one and passed validation.command="",arguments=[]) read as absent, so valid combinations were rejected and invalid ones accepted.execute()dispatched on truthiness too, so acommandthat renders to""fell through and ran aspowershell.The checks stay in
__init__. Per #70505 the constructor is the only place that can answer "was this argument passed?", so they are rewritten with theis Nonepolarity the docs now prescribe. Only the dispatch inexecute()moves tois not None, where the rendered value is finally known.The
cmdlet-derivedtask_iddefault is out of scope here. Dropping it changes task identity, so it goes in a follow-up rather than riding along with a validation fix. That also keeps this one unblocked: #70656 made the same validation fix without thetask_idchange and was closed in favour of this PR, so the burn-down now waits on this landing.PsrpOperatortherefore stays on thevalidate-operators-initexemption list, since the hook flags thetask_idderivation rather than these checks. Restoring that entry also resolved the merge conflict, which was only ever that one line.related: #70296, #70505
Testing Done
Raw logs
upstream/main'spsrp.py: