Opened 5 months ago

Closed 4 months ago

Last modified 3 months ago

#30051 closed enhancement (implemented)

Add practracker as a pre-commit and pre-push git hook for frequent coders

Reported by: teor Owned by: rl1987
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: practracker tech-debt tor-ci git-scripts tor-ci-fail-sometimes
Cc: rl1987 Actual Points:
Parent ID: #29792 Points: 1
Reviewer: asn Sponsor: Sponsor31-can

Description

  • it might still be annoying, but it's less annoying than having CI fail
  • we can fix issues for infrequent coders at the CI or merge stage
  • reviewers can see the practracker diff and decide if it's reasonable

Child Tickets

Change History (15)

comment:1 Changed 5 months ago by teor

Keywords: git-scripts added; tor-git-scripts removed

comment:2 Changed 5 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:3 Changed 5 months ago by rl1987

Status: acceptedneeds_review

comment:4 Changed 5 months ago by asn

Reviewer: asn

comment:5 Changed 5 months ago by asn

This seems to work for me.

An important thing here is that this script won't abort the commit when practracker finds issues, which seems like a reasonable idea since maybe you want to fix all the practracker issues on a subsequence commit. However, this might be a problem for people using git wrappers (like me) that ignore git-commit output, since they might not notice the failure. Personally, I turned this into a pre-commit hook for this reason, but this might not work for other people.

Tim, Nick, any opinions here?

comment:6 Changed 4 months ago by asn

Status: needs_reviewmerge_ready

Marking as merge_ready. Also see comment above.

comment:7 in reply to:  5 Changed 4 months ago by teor

Status: merge_readyneeds_revision

Replying to asn:

An important thing here is that this script won't abort the commit when practracker finds issues, which seems like a reasonable idea since maybe you want to fix all the practracker issues on a subsequence commit. However, this might be a problem for people using git wrappers (like me) that ignore git-commit output, since they might not notice the failure. Personally, I turned this into a pre-commit hook for this reason, but this might not work for other people.

Tim, Nick, any opinions here?

If CI will fail when we push, git should also fail locally.

But I don't really mind *when* we fail.

Perhaps we should make it:

  • a pre-commit hook, to encourage frequent coders to write better code
  • a pre-push hook, so mergers don't push branches that will fail CI

I think some other tickets might do what I want here.

comment:8 Changed 4 months ago by teor

Summary: Add practracker as a post-commit git hook for frequent codersAdd practracker as a pre-commit and pre-push git hook for frequent coders

comment:9 Changed 4 months ago by teor

Keywords: tor-ci-fail-sometimes added
Priority: MediumHigh
Sponsor: Sponsor31-must

CI sometimes fails because we don't check practracker before pushing.

comment:10 Changed 4 months ago by rl1987

Status: needs_revisionneeds_review

comment:11 in reply to:  10 Changed 4 months ago by asn

Status: needs_reviewneeds_revision

Replying to rl1987:

https://github.com/torproject/tor/pull/967

pretty cool! Almost there, but it seems like the pre-push hook (which is the one I'd like to use) does not block the push if practracker complains. It does throw the practracker error but does not abort the push (at least in my testing with a github branch).

comment:12 Changed 4 months ago by rl1987

Status: needs_revisionneeds_review

comment:13 Changed 4 months ago by asn

Status: needs_reviewmerge_ready

Yes, great! Thanks!

comment:14 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Squashed and merged!

comment:15 Changed 3 months ago by teor

Sponsor: Sponsor31-mustSponsor31-can

These can be sponsor 31, if we want them to be.

Note: See TracTickets for help on using tickets.