Summary

Add Mercurial hook to post to ReviewBoard on push

Review Request #8554 — Created Nov. 26, 2016 and discarded July 27, 2020, 2:15 p.m.

Information

Owner

misery

Repository

RBTools

Branch

release-0.7.x

Bugs

Depends On

~~9009~~, ~~8553~~

Reviewers

Groups

rbtools

People

Description

Add Mercurial hook to post to ReviewBoard on push

The hook only allows to push if all commits have been approved in a
review requests, otherwise it rejects it and creates a review request
for the (new) commits. This is similar to "rbt post", but

1. Does not require the user to install RBTools locally.
2. Makes sure any changes pushed to the central server have been
reviewed and approved.
3. Adds references to tickets/bugs/issues, which rbt post doesn't do.
4. Automatically finds the right review request to update if there
are any new commits, based the commit ID and a date/author hash.
This does not require the user to confirm anything, which rbt post
(often) requires. This also allows the hook to recognize
rebased/amended changesets, because the date/author hash
is unchanged.

Testing Done


Created new review requests
Updated existing review requests
Tried to push not approved changesets
Pushed approved changesets
Checked matching of changesets with another date / author hash

Issues

Description	From	Last Updated
I like to replace this by rbtools stuff but I don't know how the rbtools API works. I tried to …	misery	Nov. 30, 2016, 6:47 a.m.
Col: 51 E226 missing whitespace around arithmetic operator	reviewbot	Nov. 30, 2016, 6:47 a.m.
Should probably just be "/usr/bin/env python" (which is defined to be python 2).	david	Jan. 2, 2017, 11:48 p.m.
Should probably just be "/usr/bin/env python" (which is defined to be python 2).	david	Jan. 2, 2017, 11:49 p.m.
"Review Board"	david	Jan. 2, 2017, 11:49 p.m.
"Review Board"	david	Jan. 2, 2017, 11:49 p.m.
Typo: changset	david	Jan. 2, 2017, 11:49 p.m.
It's much more correct to actually return 0 or 1 (or some other exit code number), rather than True or …	david	Jan. 2, 2017, 11:53 p.m.
This method doesn't seem to actually return anything??	david	Jan. 2, 2017, 11:54 p.m.
This could be: return any( r.summary == request.summary for r in revreqs )	david	Jan. 2, 2017, 11:55 p.m.
If this is similar to the methods in logging, you can pass the format argument in as an argument to …	david	Jan. 3, 2017, 12:13 a.m.
In order to be consistent with the other log messages, this should probably be "Ignoring..."	david	Jan. 2, 2017, 11:56 p.m.
"None" is probably more appropriate (and less likely to cause subtle bugs) than -1.	david	Jan. 2, 2017, 11:57 p.m.
Blank line between these two.	david	Jan. 2, 2017, 11:57 p.m.
a reviewboard hook information?	david	Jan. 3, 2017, 1:18 a.m.
Blank line after class docstrings.	david	Jan. 2, 2017, 11:58 p.m.
I don't understand what this is supposed to be testing. It looks like it just processes a constant and checks …	david	Jan. 3, 2017, 1:14 a.m.
This seems like something that should be in a separate change.	david	Jan. 2, 2017, 11:58 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

contrib/tools/mercurial_push.py (Diff revision 1)

Show all issues

Col: 51
 E226 missing whitespace around arithmetic operator

contrib/tools/mercurial_push.py (Diff revision 1)

Show all issues

I like to replace this by rbtools stuff but I don't know how the rbtools API works.

I tried to derive Command to get an api_root with an authenticated session. But I get an error for this. Can someone help? :-)

            class MercurialHookCmd(Command):
                name = 'MercurialHook'
                option_list = [
                    Command.server_options,
                ]
            cmd = MercurialHookCmd()
            # cmd.run_from_argv(['','','','',''])
            server_url = cmd.get_server_url(None, None)
            api_client, api_root = cmd.get_api(url)
            get_authenticated_session(api_client, api_root, auth_required=True, num_retries=0)
            self.root = api_root





AttributeError: 'MercurialHookCmd' object has no attribute 'options'

david Nov. 30, 2016, 6:54 p.m.

The "Command" class is when adding new "rbt" commands for user interaction. If you're just making use of the RBTools API from a script, it's better to just use the API directly (by instantiating RBClient directly with a configured server name, username, and password). You could use the existing code in the contrib/tools/ and rbtools/hooks/ directories as a reference, or there are docs at https://www.reviewboard.org/docs/rbtools/0.7/api/#rbtools-api

misery Dec. 1, 2016, 12:49 a.m.

Yeah, it's a little bit bad design to use a Command for parse the configuration file.
As you see in the inter-diff I already replaced own configuration parsing with this one. I really like to you existing configuration parser of rbtools because of possible useful configs like ENABLE_PROXY, DISABLE_SSL_VERIFICATION and something like that - that I don't want to duplicate it in my code. Maybe the config-parser should be extracted from "Command" to his own class?

chipx86 Dec. 1, 2016, 2:40 a.m.

I'd be fine extracting the configuration support. I think that'd be best as its own independent change, with unit tests, so it can be considered independent of the rest of this work. Would help keep both changes smaller and more easily reviewable.

misery Dec. 22, 2016, 1:46 a.m.

Well, I looked into it. If I extract the OptionGroup classes it is vague how to handle the "server_options". What do you expect in the extracted Option class? Defined options for a command like land should still be defined in land.py. But If I extract server_options only it is a little bit confusing.

Maybe make it simple and create a "hook.py" command as a special command for all hooks?

Summary:

-	A Mercurial hook to post to ReviewBoard on push
+	Add Mercurial hook to post to ReviewBoard on push

Description:

~		A Mercurial hook to post to ReviewBoard on push
	~	Add Mercurial hook to post to ReviewBoard on push

		The hook only allows to push if all commits have been approved in a
		review requests, otherwise it rejects it and creates a review request
		for the (new) commits. This is similar to "rbt post", but

		1. Does not require the user to install RBTools locally.
		2. Makes sure any changes pushed to the central server have been
		reviewed and approved.
		3. Adds references to tickets/bugs/issues, which rbt post doesn't do.
		4. Automatically finds the right review request to update if there
		are any new commits, based the commit ID and a date/author hash.
		This does not require the user to confirm anything, which rbt post
		(often) requires. This also allows the hook to recognize
		rebased/amended changesets, because the date/author hash
		is unchanged.

Testing Done:

	+	Created new review requests
	+	Updated existing review requests
	+	Tried to push not approved changesets
	+	Pushed approved changesets
	+	Checked matching of changesets with another date / author hash

Commit:

-	efa27376c51823355d58b93bde602ba9e0ba4c7b
+	f3e2978c0d8ee62e97398c9fa6449826ca413eb4

Diff:

Revision 2 (+535 -2)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/utils/review_request.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/review_request.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

Change Summary:

Fix encoding again and use HookError exception

Commit:

-	f3e2978c0d8ee62e97398c9fa6449826ca413eb4
+	0a9341082aa8f72f6c31059a868f5773c7a27b14

Diff:

Revision 3 (+537 -2)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/utils/review_request.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/review_request.py
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

New code, even that in contrib/, should use the new docstring conventions: https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/

contrib/tools/mercurial_push.py (Diff revision 3)

Show all issues

Should probably just be "/usr/bin/env python" (which is defined to be python 2).

contrib/tools/mercurial_push.py (Diff revision 3)

Show all issues

Should probably just be "/usr/bin/env python" (which is defined to be python 2).

contrib/tools/mercurial_push.py (Diff revision 3)
Show all issues
```
"Review Board"
```
contrib/tools/mercurial_push.py (Diff revision 3)
Show all issues
```
"Review Board"
```
contrib/tools/mercurial_push.py (Diff revision 3)
Show all issues
```
Typo: changset
```

contrib/tools/mercurial_push.py (Diff revision 3)

Show all issues

It's much more correct to actually return 0 or 1 (or some other exit code number), rather than True or False.

contrib/tools/mercurial_push.py (Diff revision 3)
Show all issues
```
This method doesn't seem to actually return anything??
```

contrib/tools/mercurial_push.py (Diff revision 3)

Show all issues

This could be:

return any(
    r.summary == request.summary
    for r in revreqs
)

contrib/tools/mercurial_push.py (Diff revision 3)

Show all issues

If this is similar to the methods in logging, you can pass the format argument in as an argument to log(), which prevents trying to format it twice.

contrib/tools/mercurial_push.py (Diff revision 3)

Show all issues

In order to be consistent with the other log messages, this should probably be "Ignoring..."

contrib/tools/mercurial_push.py (Diff revision 3)

Show all issues

"None" is probably more appropriate (and less likely to cause subtle bugs) than -1.

contrib/tools/mercurial_push.py (Diff revision 3)
Show all issues
```
Blank line between these two.
```

rbtools/hooks/mercurial.py (Diff revision 3)

Show all issues

a reviewboard hook information?

misery Jan. 3, 2017, 1:18 a.m.

Yeah, it is possible for a Mercurial changeset to have no diff. Like a new branch without changes or close a branch. To avoid failing to create a review request for those changesets we add a "fake diff" with the pure raw information of that changeset and shows it to the reviewer. But of course.... the review needs to know that this is a special INFORMATION and not a real diff.

rbtools/hooks/mercurial.py (Diff revision 3)
Show all issues
```
Blank line after class docstrings.
```

rbtools/hooks/tests.py (Diff revision 3)

Show all issues

I don't understand what this is supposed to be testing. It looks like it just processes a constant and checks how many lines in that constant start with + but not +++. That doesn't seem like a very useful thing to test.

misery Jan. 3, 2017, 1:18 a.m.

Well, the test description should show that. In _generate_diff_info it uses the magic number 5 to avoid useless counting for every review request. So if someone changes the FAKE_DIFF_TEMPL he needs to adjust that magic number 5. This test is to ensure that no one will forget that.

rbtools/utils/review_request.py (Diff revision 3)

Show all issues

This seems like something that should be in a separate change.

Commit:

-	0a9341082aa8f72f6c31059a868f5773c7a27b14
+	e68ca96568d04a3ad8a895de1294051ca065f32d

Diff:

Revision 4 (+744 -1)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/clients/mercurial.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/hooks/mercurial.py
    contrib/tools/mercurial_push.py
    rbtools/clients/mercurial.py
    rbtools/hooks/tests.py

misery Feb. 18, 2017, 1:32 p.m.
```
Some chance to get it in? :-)
```
misery May 3, 2017, 12:40 p.m.
```
ping?
```

misery June 12, 2017, 3:51 p.m.

Well, if I move everything into the contrib file. Will it be accepted?

Extracted: https://reviews.reviewboard.org/r/9009/

Depends On:

+	~~9009 - Create better diffs from mercurial~~

Change Summary:

rebase to merged r9009

Commit:

-	e68ca96568d04a3ad8a895de1294051ca065f32d
+	16a5225e9f75426016dc79728fd9449c34337073

Diff:

Revision 5 (+743)

Show changes

	contrib/tools/mercurial_push.py
	rbtools/hooks/mercurial.py
	rbtools/hooks/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Do you accept it if I move rbtools/hook stuff to contrib/tools/mercurial_push.py ? :-)

I improved the script again and moved this to contrib of my ExtendedApproval extension.

I really like to see this in rbtools someday.

https://github.com/misery/ExtendedApproval/blob/master/contrib/mercurial_push.py

Status: Discarded

Change Summary:

https://github.com/misery/ExtendedApproval/blob/master/contrib/mercurial_git_push.py