SEPURI-SAI-KRISHNA commented on issue #72144: URL: https://github.com/apache/airflow/issues/72144#issuecomment-5440131327
Thanks, the EMR catch is real and I've fixed it. **`EmrContainerOperator` / `EmrContainerSensor`:** you're right, and the miss was worse than one site each. Both build the trigger inside a conditional expression, so there are *two* constructions per site, four in total, all passing none of the three. My sweep only looked at `trigger=<Call>` and skipped `ast.IfExp` entirely, which means my invariant test would have skipped them too. The test now resolves the `trigger=` expression to every construction it can evaluate to, and a defer site whose trigger is a bare reference is asserted against an explicit allowlist rather than silently ignored. That last part catches one more: `EksPodOperator` passes a pre-built `EksPodTrigger`, which is a `KubernetesPodTrigger` and out of scope. `EmrContainerTrigger` also needed its hook's `virtual_cluster_id`, so it overrides `_hook_parameters` rather than relying on the default, which is the escape hatch the base class keeps for exactly this. **`SageMakerNotebookOperator`:** agreed, excluded. I'd reached the same conclusion from the trigger side, `SageMakerNotebookJobTrigger` is a plain `BaseTrigger` whose hook is addressed by execution name, and your reason from the operator side is the same boundary. **Ordering / crash risk:** agreed, and it's why this is one PR rather than a series. The trigger signatures and the call sites move together, so there is no intermediate commit where an operator forwards a parameter its trigger cannot accept. **Signature count:** we're measuring different things, and your number is the right one for classes. My 54 was *defer sites* that could not be fixed at the call site. Counting classes instead: of the 45 this PR touches, 31 accept none of the three, 12 already accept `region_name`, and 2 accept some other subset. So ~30 fully closed is correct; I should have said "sites" more clearly in the issue. Net effect after the fix: 113 of 114 trigger constructions forward the hook configuration, the remaining one being the SageMaker case above. -- 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]
