Summary:

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

Review Request #5596 — Created March 7, 2014 and submitted March 29, 2014, 11:46 p.m.

Information
Owner:	anselina
Repository:	Review Board
Branch:	master
Bugs:
Depends On:	~~5510~~, ~~5603~~
Blocks:	5653
Commit:	798641b...
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

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.

Files

Issues

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	March 23, 2014, 12:25 p.m.
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	March 23, 2014, 12:26 p.m.
Blank line at the end	B. b.ramnani	March 22, 2014, 4:06 p.m.
A blank line below this last line.	B. b.ramnani	March 22, 2014, 9:47 p.m.
We generally list full-package imports ahead of single-item imports, so the from one should go after import logging.	david	March 25, 2014, 7:28 p.m.
How about we turn these into ints?	david	March 25, 2014, 9:07 p.m.
There's an extra blank line here.	david	March 25, 2014, 7:29 p.m.
There's an extra blank line here.	david	March 25, 2014, 7:29 p.m.
Can you wrap this in parentheses instead of using the character?	david	March 25, 2014, 9:44 p.m.
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	March 29, 2014, 11:55 a.m.

Change Summary:

Added a fix for closing private review requests.

Description:

		This is the initial implementation of the 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>".

		It currently works for simple use cases (pushing a commit that references /r/<id> automatically closes that review request with the proper change description, etc.).

		TODO:
		- Test/handle more complex cases (merge commits, branch creation, etc.).
-		- Add a fix so the hook can close private review requests.
		- Refactor.

Testing Done:

		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 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)

Depends On:

+	~~5603 - Split up _find_review_request to retrieve the ReviewRequest object, then check permissions.~~

Commit:

-	9bab2d4730596769fcf6c7e418243e348ba84a6c
+	b7c3a5a42367ef462cdda57a8d9251e89c57503c

Diff:

Revision 2 (+138)

Show changes

	reviewboard/urls.py
	reviewboard/hostingsvcs/hooks.py

Description:

~		This is the initial implementation of the 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>".
	~	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>".
-
-		It currently works for simple use cases (pushing a commit that references /r/<id> automatically closes that review request with the proper change description, etc.).

		TODO:
-		- Test/handle more complex cases (merge commits, branch creation, etc.).
		- Refactor.

Testing Done:

		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)
	~	- 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

Commit:

-	b7c3a5a42367ef462cdda57a8d9251e89c57503c
+	0d3bb7bd5afcd2f96cb72b0c6b3589eb09540131

Diff:

Revision 3 (+144)

Show changes

	reviewboard/urls.py
	reviewboard/hostingsvcs/hooks.py

Added Files:

Description:

		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>".

		TODO:
~		- Refactor.
	~	- Make the regex customizable.

Commit:

-	0d3bb7bd5afcd2f96cb72b0c6b3589eb09540131
+	82f1c89dcf6b2129647c45e28c7e3bb691f51457

Diff:

Revision 4 (+145)

Show changes

	reviewboard/urls.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

Change Summary:

Made the regex customizable.

Summary:

-	[WIP] Add GitHub post-receive hook to close review requests automatically.
+	Add GitHub post-receive hook to close review requests automatically.

Description:

~		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>".
	~	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.
-
-		TODO:
-		- Make the regex customizable.

Testing Done:

		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.

Commit:

-	82f1c89dcf6b2129647c45e28c7e3bb691f51457
+	e962798b51c6c7d49d9785a77d951818fd0d6025

Diff:

Revision 5 (+157)

Show changes

	reviewboard/settings.py
	reviewboard/urls.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

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.

anselina March 22, 2014, 2:34 p.m.

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

chipx86 March 23, 2014, 11:43 a.m.

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.

anselina March 23, 2014, 12:26 p.m.

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

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...

reviewboard/hostingsvcs/hook_utils.py (Diff revision 5)

Show all issues

A blank line below this last line.

anselina March 22, 2014, 2:34 p.m.

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

b.ramnani March 22, 2014, 4:07 p.m.

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

anselina March 22, 2014, 9:19 p.m.

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

reviewboard/hostingsvcs/github.py (Diff revision 5)
Show all issues
```
Blank line at the end
```

Commit:

-	e962798b51c6c7d49d9785a77d951818fd0d6025
+	b1865a09154df6a9343c17e54c952ca35f1c5c44

Diff:

Revision 6 (+157)

Show changes

	reviewboard/settings.py
	reviewboard/urls.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

Commit:

-	b1865a09154df6a9343c17e54c952ca35f1c5c44
+	3fef25d52185f7b50842fa94fa3d6d856c754cfa

Diff:

Revision 7 (+159)

Show changes

	reviewboard/settings.py
	reviewboard/urls.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

Change Summary:

Defined the hook URL in github.py instead (depends on /r/5510).

Depends On:

+	~~5510 - SCM Web Hooks for reviewboard : Phase 1~~

Commit:

-	3fef25d52185f7b50842fa94fa3d6d856c754cfa
+	f618eafe99d97e81914aa313e671771558b3f0ad

Diff:

Revision 8 (+162)

Show changes

	reviewboard/settings.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

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.

reviewboard/hostingsvcs/github.py (Diff revision 8)
Show all issues
```
How about we turn these into ints?
```
reviewboard/hostingsvcs/github.py (Diff revision 8)
Show all issues
```
There's an extra blank line here.
```
reviewboard/hostingsvcs/hook_utils.py (Diff revision 8)
Show all issues
```
There's an extra blank line here.
```

Change Summary:

Changed all review request ID variables to ints, and fixed style issues.

Commit:

-	f618eafe99d97e81914aa313e671771558b3f0ad
+	2ea55e6987331e35edb068e2a104fcf464479104

Diff:

Revision 9 (+158)

Show changes

	reviewboard/settings.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

reviewboard/settings.py (Diff revision 9)

Show all issues

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

anselina March 25, 2014, 9:45 p.m.

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

Commit:

-	2ea55e6987331e35edb068e2a104fcf464479104
+	c673359f58c09014987475e8a977a971631cad1c

Diff:

Revision 10 (+157)

Show changes

	reviewboard/settings.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

Ship it!

This looks good to me, pending the webhooks URL change going in.

Change Summary:

Added a try/except block to make sure the payload is in JSON format before processing it (since GitHub also supports x-www-form-urlencoded).

Commit:

-	c673359f58c09014987475e8a977a971631cad1c
+	798641bb69e2e86adc690cc634df186f3e53331f

Diff:

Revision 11 (+162)

Show changes

	reviewboard/settings.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/hook_utils.py

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 ?

anselina March 29, 2014, 2:01 a.m.

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>"?

anselina March 29, 2014, 11:55 a.m.

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.)

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (3974915). Thanks!