SEPURI-SAI-KRISHNA commented on PR #72171:
URL: https://github.com/apache/airflow/pull/72171#issuecomment-5612323502
Thanks, this is a genuinely useful review. Addressed 1, 4 and 5 in the
commit above; concrete answers to 2 and 3 below.
### 1. Failure mode moved into the triggerer: fixed
You are right, and `EksDeleteClusterTrigger` bypassing `super().__init__()`
is exactly why I put the check in `__init_subclass__` rather than `__init__`.
It now fails when the class is created, so an unusable trigger cannot be
imported, let alone deferred to.
The check accepts a subclass that declares `aws_hook_class` **or** overrides
`hook()`, and it tests the override as `cls.hook is not
AwsBaseWaiterTrigger.hook`, so an override inherited from an intermediate base
still counts. It also asserts `issubclass(..., AwsGenericHook)` as you asked,
so a typo'd assignment now says so instead of raising `TypeError` from calling
a non-class. That check needs `AwsGenericHook` at runtime, so its import moves
out of the `TYPE_CHECKING` block. It is the same module-level import
`utils/mixins.py` already carries, and nothing in `hooks/base_aws.py` imports
triggers, so there is no cycle. Four tests in `triggers/test_base.py` cover
both rejections and both accepted shapes. Every existing trigger imports clean.
### 2. Moving the static half to a prek hook: agreed, but as a follow-up
I have fixed the three that are real bugs regardless of where the code lives:
- `.as_posix()` instead of `str(path.relative_to(...))`, so the allowlist
tuples keep matching on Windows.
- `find_hand_built_hooks` now matches an `ast.Attribute` callee, so a
`module.SomeHook(...)` construction cannot slip past. It finds no new sites
today; it closes the hole.
- The `"hook" in vars(...)` check is now the MRO-aware comparison described
above.
On the move itself, I would rather do it in a follow-up than here, for two
reasons. This PR is already 33 files, and the invariant is the thing that makes
the rest of them reviewable, so changing where it lives in the same diff means
the evidence and the change move together. And a prek hook needs a new script
plus a `.pre-commit-config.yaml` entry, which is a different review from a
provider migration. I will open it once this lands.
The duplicated `self.defer` matching and the bare-filename keys in
`HAND_BUILT_HOOK_EXCEPTIONS` are both worth doing and I will fold them into
that follow-up.
On the collection errors: I do not think `find_waiter_triggers()` is the
cause. That file collects and runs clean here, 244 passed and 5 skipped. At
this commit the full operator, sensor and trigger run is 2528 passed, 5
skipped, 0 failures, 29 collection errors, and every one of the 29 is
`ModuleNotFoundError: No module named 'airflow_shared'` in files this PR does
not touch. I have corrected the stale counts in the description, which is where
the "30" you saw came from.
Worth flagging that moving the four AST tests to prek would not remove the
import requirement either, since `test_waiter_trigger_can_build_a_hook`, the
one you suggested keeping as a unit test, is precisely the one that imports
every trigger module. If we want that robust for partial installs it needs its
own guard, which I will include in the follow-up.
### 3. Merge order
**#72098 -> this -> #72449 / #72472, those last two in either order.** It is
less entangled than it looks, and I should have said so in the description.
**#72098 to this is the only hard dependency, and it is containment rather
than conflict.** This PR carries a rebased copy of #72098's two commits, and
all 8 of its files are a strict subset of the 33 here. So #72098 can merge
whenever it is ready on its own merits, and when it does, this PR simply gets 8
files smaller on the next sync. Nothing to coordinate.
**This PR has no file-level overlap with either workshop PR.** It does not
touch `sensors/batch.py`, `sensors/opensearch_serverless.py` or
`triggers/opensearch_serverless.py`, and neither workshop PR touches the test
file here. There is no merge conflict in any ordering.
The whole coupling is three lines in one allowlist:
| PR | line to delete when it merges second |
|---|---|
| #72449 | `("sensors/batch.py", "BatchJobTrigger")` from
`PENDING_MIGRATION` |
| #72472 | `("sensors/opensearch_serverless.py",
"OpenSearchServerlessCollectionActiveTrigger")` from `PENDING_MIGRATION`
**and** `("opensearch_serverless.py", "OpenSearchServerlessHook")` from
`HAND_BUILT_HOOK_EXCEPTIONS` |
It is symmetric, and the assertion prints the exact line to remove, so it is
a one-line fix with no investigation. To keep it off your plate: **I will watch
all three and push the deletion myself**, wherever it needs to land. Merge them
in whatever order suits.
### 4. Description: fixed
You are right that it contradicted itself. Only two services are in
`PENDING_MIGRATION`, and the paragraph immediately below explains why SageMaker
is not an instance of this bug at all. Rewritten so the workshop list is the
two that are actually allowlisted, with SageMaker and Eks described only as the
not-applicable cases they are. Merge order added.
### 5. Changelog: added
Added a `.. warning::` at the top of `providers/amazon/docs/changelog.rst`,
alongside the existing ones, since that block is the hand-written part the
release manager does not regenerate. It names what changes observably: the
triggerer now uses the operator's region instead of the triggerer host's
`AWS_DEFAULT_REGION`, and applies the SSL verification and botocore config that
never reached it before.
### Minor
Agreed on all three, and none touched here. `EksCreateClusterTrigger`'s
duplicate `region_name` and `EksDeleteClusterTrigger` skipping
`super().__init__()` are both pre-existing and both a good fit for the
follow-up, now that the base owns more of this. Leaving the docstring wording
variance alone as you suggested.
---
Drafted-by: Claude Code (Opus 5); reviewed by @SEPURI-SAI-KRISHNA 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]
