-
-
Notifications
You must be signed in to change notification settings - Fork 123
Implement check-hook-updates builtin hook
#1249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1249 +/- ##
==========================================
- Coverage 91.06% 90.96% -0.10%
==========================================
Files 87 88 +1
Lines 18384 18516 +132
==========================================
+ Hits 16741 16843 +102
- Misses 1643 1673 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c06e5ce to
bf09775
Compare
📦 Cargo Bloat ComparisonBinary size change: +0.44% (22.8 MiB → 22.9 MiB) Expand for cargo-bloat outputHead Branch ResultsBase Branch Results |
bf09775 to
45b59a4
Compare
|
Is it possible to eliminate the code duplication from |
45b59a4 to
4da4b05
Compare
Good call. Done! |
4da4b05 to
37c2e0f
Compare
|
Apologies - my previous commit didn't actually eliminate the duplication as I claimed. I've now properly extracted the shared logic into reusable functions in
|
| fn check_hook_updates_hook_recognized() { | ||
| let context = TestContext::new(); | ||
| context.init_project(); | ||
|
|
||
| context.write_pre_commit_config(indoc::indoc! {r" | ||
| repos: | ||
| - repo: builtin | ||
| hooks: | ||
| - id: check-hook-updates | ||
| "}); | ||
| context.git_add("."); | ||
|
|
||
| // The hook should be recognized and run (it will pass since there are no remote repos to check) | ||
| cmd_snapshot!(context.filters(), context.run(), @r" | ||
| success: true | ||
| exit_code: 0 | ||
| ----- stdout ----- | ||
| check for hook updates...................................................Passed | ||
| ----- stderr ----- | ||
| "); | ||
| } | ||
|
|
||
| /// Tests that `check-hook-updates` hook with `--fail-on-updates` argument is recognized. | ||
| #[test] | ||
| fn check_hook_updates_hook_with_args() { | ||
| let context = TestContext::new(); | ||
| context.init_project(); | ||
|
|
||
| context.write_pre_commit_config(indoc::indoc! {r" | ||
| repos: | ||
| - repo: builtin | ||
| hooks: | ||
| - id: check-hook-updates | ||
| args: ['--cooldown-days=7', '--fail-on-updates'] | ||
| "}); | ||
| context.git_add("."); | ||
|
|
||
| // The hook should be recognized and run with the arguments | ||
| cmd_snapshot!(context.filters(), context.run(), @r" | ||
| success: true | ||
| exit_code: 0 | ||
| ----- stdout ----- | ||
| check for hook updates...................................................Passed | ||
| ----- stderr ----- | ||
| "); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think these two tests are enough, because there aren’t any remote repos in the config file, so the update logic never actually runs. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You, sir, are correct again.
Just added tests that exercise the update-checking logic with a real remote repo (pre-commit/pre-commit-hooks v4.0.0):
check_hook_updates_detects_outdated_repo: verifies hook runs and detects updatescheck_hook_updates_fails_on_updates: verifies --fail-on-updatescorrectly fails when updates are available
|
|
@pygarap
|
87a28cb to
f8af851
Compare
|
When would this hook run? I tried to understand the "files" arg but couldn't see it in the diff, and I believe it will run everytime prek is triggered. This could cause the hook to run when not needed and generate noise. I wouldn't want this hook to complain about my git hook setup when the only thing I'm doing is editing an unrelated code file. |
15d1efc to
11589d7
Compare
@ulgens Great point! I've addressed this by adding a check interval feature that throttles how often the hook actually runs. ChangesNew default behavior:
New arguments:
Does this address your concerns adequately or do you have better ideas/further suggestions? Example usagerepos:
- repo: builtin
hooks:
- id: check-hook-updates
# Check at most once per day, only suggest releases older than 7 days
# (these are the defaults, shown for clarity)
args: ['--check-interval-hours=24', '--cooldown-days=7']For CI where you want every-run checks: - id: check-hook-updates
args: ['--check-interval-hours=0', '--fail-on-updates']Open questionOne thing I noticed: when the check is skipped due to the interval, the hook still shows "Passed" rather than "Skipped". This could be slightly misleading since no actual check occurred. Would it be better to show "Skipped" in this case? I'd need to look into how the hook result reporting works to see if this is feasible without larger changes. Open to suggestions! |
I don't have a strong opposition to this implementation, and one could easily not enable the hook if they don't like it, but it feels a lot like this should be a step/layer in the CI , not a git hook. git hooks are generally (always?) invoked based on the repository state and files, not a pre-defined interval. In contrast, running a weekly task on CI feels much more natural. |
|
That's a fair point! You're right that git hooks are traditionally file/state-based, and a time-based interval is unconventional. A few thoughts on why this might still be useful as a hook:
That said, I'm open to alternative approaches. If the time-based behavior feels too unconventional, users can simply set |
11589d7 to
f9f1c50
Compare
Add a new builtin hook that checks if configured hooks have newer versions available. Supports: - `--cooldown-days`: minimum release age for eligible updates - `--fail-on-updates`: fail instead of just warning Closes j178#1243
…te duplication Extract shared functionality from auto_update.rs that check_hook_updates can reuse: - `find_eligible_tag`: cooldown-based tag selection with best candidate logic - `resolve_rev_to_commit_hash`: dereference a revision to its commit hash This eliminates duplicated code for: - Cooldown calculation and binary search for eligible tags - Git rev-parse logic for resolving revisions to commit hashes
Add tests that actually exercise the update-checking logic by using a real remote repo (pre-commit/pre-commit-hooks) with an outdated version (v4.0.0). - check_hook_updates_detects_outdated_repo: verifies hook runs with remote repos - check_hook_updates_fails_on_updates: verifies --fail-on-updates works
f9f1c50 to
27a96ad
Compare
- Default to checking at most once every 24 hours to avoid slowing commits - Store last check timestamp in prek cache directory - Change cooldown-days default from 0 to 7 for supply chain security - Add --check-interval-hours=0 to force check every time (useful for CI) - Update docs with new arguments and security rationale - Add tests for check interval behavior Addresses review feedback about hook running on every commit.
27a96ad to
ff56f40
Compare
37908ff to
6b3673c
Compare
Add a new builtin hook that checks if configured hooks have newer versions available.
Features
--cooldown-days: minimum release age (in days) for a version to be eligible (default: 0)--fail-on-updates: fail the hook if updates are available (default: warn only)Example usage
Changes
CheckHookUpdatesvariant toBuiltinHooksenumpre_commit_hooks/check_hook_updates.rs.pre-commit-config.yamlCloses #1243