SEPURI-SAI-KRISHNA commented on PR #73681: URL: https://github.com/apache/airflow/pull/73681#issuecomment-5819559384
Adding a file under `scripts/` pulled in the dev tools owners, so here are the three decisions in this PR that are mine rather than obvious, and two questions I would rather ask than guess at. **Why the hook is scoped to one provider.** The parameters it checks, `region_name`, `verify` and `botocore_config`, are the boto3 client settings, so the check cannot be widened to other providers without becoming a per provider configuration of parameter names and allowlists. The same class of bug does exist elsewhere, Google deferrable operators drop `impersonation_chain` the same way, but a generic version is a different and larger design than this one. `check-common-ai-operators-index` is already scoped to a single provider, so I followed that rather than invent a pattern. If you would rather this lived somewhere other than `scripts/ci/prek/`, say where and I will move it. **Why `pass_filenames: false`.** The allowlists are checked in both directions: an entry that is no longer needed fails the check and has to be removed. That only works with a view of the whole provider, so the hook ignores which files changed and always sweeps. The cost is 1.08s over the 276 files in the provider. I did not add caching for that, since the repeated parse is cheap and a cache would be one more thing to reason about. **A limitation that is unchanged, not introduced.** The hand built hook sweep matches a callee whose name ends in `Hook`, so a hook created through a variable or an attribute is not seen. That was already the case in the test this replaces and was noted in the review on #72171. Fixing it needs a different approach than name matching, so it is not in scope here. On the diff shape: the 228 deleted lines are the four static checks moving, not coverage going away. The hook and the test it replaces were run against the same tree and agree exactly, 116 defer sites and 47 hand built hook constructions, and each of the 26 new tests was checked against a deliberately broken hook so that none of them pass by accident. Two questions. Is `scripts/ci/prek/` the right home for a check that only ever looks at one provider, or would you prefer provider scoped checks to live under the provider itself? I have no attachment to the location. This picked up `backport-to-v3-3-test` automatically. The hook and its tests are not shipped in the provider distribution, so I do not think it needs backporting, but I would rather a maintainer confirm than remove a label myself. -- 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]
