SEPURI-SAI-KRISHNA commented on PR #72098:
URL: https://github.com/apache/airflow/pull/72098#issuecomment-5437099492
Thanks for the review, both addressed.
**`verify` in the docstrings:** added to all seven classes in
`neptune_analytics.py`, using the wording already standard elsewhere in
the provider. Two of them (`NeptuneDeletePrivateGraphEndpointOperator`
and `NeptuneDeleteGraphOperator`) had a stray blank line sitting exactly
where the entry belonged, so the parameter looks like it was dropped
during authoring rather than deliberately omitted.
**Serialized assertions:** switched all twelve tests to
`trigger.serialize()[1]` with a full payload comparison, matching
`test_neptune.py`. You're right that this is the stronger check —
`serialize()` runs the values through `prune_dict`, so an attribute
assertion can pass while the setting never reaches the triggerer. It also
caught something the attribute form hid: the MWAA sensors declare
`success_states` / `failure_states` as lists but serialize them as sets.
I re-ran the counterfactual after the rewrite: stripping the 36 fix lines
still fails exactly the 12 new tests and nothing else, so the sensitivity
is unchanged.
On the broader points, all three sound right to me:
1. I have the full audit already, 84 sites originally, 19 now fixed
across #71646, #71857 and this PR. My grouping matches your count
almost exactly (18 passing only `region_name`; I count 44 rather than
43 passing none). Happy to open a tracking issue with the per-service
breakdown.
2. Agreed, and this is the more interesting fix. A default `hook()` on
`AwsBaseWaiterTrigger` driven by a `hook_class` attribute would cover
the group that can't be fixed at the call site, and a parametrized
invariant test over the deferrable operators would stop new ones
regressing. I'd rather do that as its own PR than fold it in here.
3. Makes sense, I'll move the constants to a shared helper as part of
that work rather than copy them a thirteenth time.
Let me know if you'd prefer the tracking issue before or after the
`AwsBaseWaiterTrigger` change.
--
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]
