rusackas opened a new pull request, #42044: URL: https://github.com/apache/superset/pull/42044
### SUMMARY The `pre-commit checks` job is a required status check, yet lint/type violations have been reaching `master` in files the linters never actually ran on (recently a `ruff` `C901` complexity error in `execute_sql.py` and a `mypy` typing error in the semantic-layers tests, both cleaned up after the fact in #42014). Root cause is in the job itself, not the hooks: 1. **It fails open.** The step runs `pre-commit run --files $CHANGED_FILES` under `set +e`. When the changed-files list is empty, `pre-commit run --files` with no files is a no-op that exits `0` — so the required check reports success having linted nothing. 2. **The file list came from a dead action.** `steps.changed_files` used [`trilom/file-changes-action`](https://github.com/trilom/file-changes-action) — archived, last released in 2020, pinned as a git submodule — which is known to silently return empty/partial lists. Combined with (1), any hiccup there turned the whole gate into a silent pass. #### What this PR changes **Commit 1 — fail closed, drop the archived action.** Replace the third-party action with a small, dependency-free `git diff` against the PR/push base and make the empty case explicit: - Diff base unresolvable → run `--all-files` (never "nothing"). - Genuinely empty diff → pass explicitly (`mode=none`). - Otherwise → `pre-commit run --files <changed>` (unchanged behavior). `fetch-depth: 0` is added so the base commit is available for the diff, and the now-unused `file-changes-action` submodule is removed (it was referenced only in this workflow). **Commit 2 — nightly all-files backstop.** Per-PR runs only lint changed files, which is fundamentally leaky for whole-program checkers like `mypy`: a PR that changes a type in module A never re-checks the tests/modules that import A. A scheduled daily `pre-commit run --all-files` catches that cross-file drift. This is defense-in-depth and can be dropped independently if the nightly signal isn't wanted. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A — CI workflow change. ### TESTING INSTRUCTIONS The changed-files detection logic was exercised locally across every branch: | Event | Base | Expected | Result | |---|---|---|---| | `schedule` | — | `mode=all` | ✅ | | `pull_request` | real base | `mode=files` (correct list) | ✅ | | `push` | all-zero SHA | fail-closed `mode=all` | ✅ | | `push` | bogus SHA | fail-closed `mode=all` | ✅ | | `pull_request` | base == HEAD | `mode=none` | ✅ | Also validated: `yaml` parses, `zizmor` reports no findings, and `github.event`-derived SHAs are passed via `env:` (no template injection). Reviewers can confirm on this PR that the job scopes to the changed workflow file (not `--all-files`), proving the changed-files path still works. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Introduces new feature or API - [ ] Removes existing feature or API 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
