Add GitHub post-receive hook to close review requests automatically.

Review Request #5596 — Created March 7, 2014 and submitted

Information

Review Board
master
798641b...

Reviewers

This is a GitHub post-receive hook that will automatically close review requests as "submitted" after a push. To determine which review requests should be closed, it scans through each commit's commit message for the following strings (case-insensitive): "Reviewed at <reviewboard_url>/r/<id>" or "Review request #<id>". The regex used for this can be overriden in settings_local.py.

Added the post-receive webhook URL to a private GitHub repository, and tested different pushes:
- Commit with a review request ID that is not submitted (verified that the review request is closed and set to submitted)
- Commit with a review request ID that is already submitted (got a warning that the review request is already submitted)
- Commit without a review request ID in the commit message (got a debug message that no matching review request ID was found)
- Commit with a private review request (verified that the review request is closed)
- Commits referencing the same review request ID
- Multiple commits
- Merge commits
- Branch creation and deletion
- Commits pushed to two branches

I also overrode the regex and flags in settings_local.py, and got the expected change in matching review request IDs.


Description From Last Updated

Should probably be a @classmethod of HostingService in service.py which can be overriden by individual services like Github and Bitbucket. …

B. b.ramnani

Same can be said about this method. This should probably be a private method. i.e. renamed to _get_review_id_to_commits_map...

B. b.ramnani

Blank line at the end

B. b.ramnani

A blank line below this last line.

B. b.ramnani

We generally list full-package imports ahead of single-item imports, so the from one should go after import logging.

daviddavid

How about we turn these into ints?

daviddavid

There's an extra blank line here.

daviddavid

There's an extra blank line here.

daviddavid

Can you wrap this in parentheses instead of using the character?

daviddavid

This function returns "http://example.com" on my localhost, when I tested for post-receive hook for the bitbucket repo. Is that the …

B. b.ramnani
anselina
anselina
anselina
anselina
B.
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Should probably be a @classmethod of HostingService in service.py which can be overriden by individual services like Github and Bitbucket. Would increase reusability. Would also increase encapsulation, since the repository_url_patterns need to reference this view. Maybe the mentors could comment on that.

    1. Yeah, I wasn't sure about this one and get_review_id_to_commits_map(). Would be good to have input from the mentors!

    2. I think a @classmethod would make sense if it was another part of the architecture driving the process for referencing and calling the view, but in this case, it's the class itself. While there are similarities between the hook views in different classes, I don't think we want to bake in the concept in the HostingService class itself. There will be many more views in the future, and it'll be unclear what should be considered "common" enough and what shouldn't.

      Instead, I think we should keep things as they are today. A @classmethod would be fine, though, just only on the specific HostingService subclasses.

      I think having the hook_utils.py is the right way of sharing common code here.

    3. I'm keeping this and get_review_id_to_commits_map() as global functions, as discussed.

  3. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Same can be said about this method. This should probably be a private method. i.e. renamed to _get_review_id_to_commits_map...

  4. 
      
B.
  1. 
      
  2. reviewboard/hostingsvcs/hook_utils.py (Diff revision 5)
     
     
    Show all issues

    A blank line below this last line.

    1. Hm, I'm not sure why this is necessary? I don't remember reading anything about newlines at the end of files.

    2. I've observed this in all files. Maybe it's not a necessary style requirement. I'm not sure.

    3. Cool, I'll double-check and fix it if so - thanks for noticing!

  3. 
      
B.
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    Blank line at the end

  3. 
      
anselina
anselina
anselina
david
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 8)
     
     
     
     
    Show all issues

    We generally list full-package imports ahead of single-item imports, so the from one should go after import logging.

  3. reviewboard/hostingsvcs/github.py (Diff revision 8)
     
     
     
     
     
    Show all issues

    How about we turn these into ints?

  4. reviewboard/hostingsvcs/github.py (Diff revision 8)
     
     
    Show all issues

    There's an extra blank line here.

  5. reviewboard/hostingsvcs/hook_utils.py (Diff revision 8)
     
     
    Show all issues

    There's an extra blank line here.

  6. 
      
anselina
david
  1. 
      
  2. reviewboard/settings.py (Diff revision 9)
     
     
     
    Show all issues

    Can you wrap this in parentheses instead of using the character?

    1. Thanks, I always forget you can do this in Python!

  3. 
      
anselina
david
  1. This looks good to me, pending the webhooks URL change going in.

  2. 
      
anselina
B.
  1. 
      
  2. reviewboard/hostingsvcs/hook_utils.py (Diff revision 11)
     
     
    Show all issues

    This function returns "http://example.com" on my localhost, when I tested for post-receive hook for the bitbucket repo. Is that the expected behavior ?

    1. Hm, how did you test it? When you tested the post-receive hook on a Bitbucket Git repo, did you try commits with the commit message containing "Reviewed at <server_url>/r/<id>"?

    2. Dropping this because we figured out that Bhushan's RB server was not properly configured. (The "Server" field was set to http://example.com on the System Settings -> General page in the admin panel.)

  3. 
      
anselina
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (3974915). Thanks!