Add Mercurial hook to post to ReviewBoard on push

Review Request #8554 - Created Nov. 26, 2016 and updated

André Klitzing
RBTools
release-0.7.x
9009, 8553
16a5225...
rbtools

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.

  • 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
  • 0
  • 0
  • 16
  • 2
  • 18
Description From Last Updated
Review Bot
  1. 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
    
    
  2. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
    Col: 51
     E226 missing whitespace around arithmetic operator
    
  3. 
      
André Klitzing
  1. 
      
  2. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
     

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

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

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

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

  3. 
      
André Klitzing
Review Bot
  1. 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
    
    
  2. 
      
André Klitzing
Review Bot
  1. 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
    
    
  2. 
      
David Trowbridge
  1. New code, even that in contrib/, should use the new docstring conventions: https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/

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

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

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

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

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

    "Review Board"

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

    "Review Board"

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

    Typo: changset

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

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

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

    This method doesn't seem to actually return anything??

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

    This could be:

    return any(
        r.summary == request.summary
        for r in revreqs
    )
    
  10. contrib/tools/mercurial_push.py (Diff revision 3)
     
     

    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.

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

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

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

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

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

    Blank line between these two.

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

    a reviewboard hook information?

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

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

    Blank line after class docstrings.

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

    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.

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

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

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

  18. 
      
André Klitzing
Review Bot
  1. 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
    
    
    1. Some chance to get it in? :-)

    2. ping?

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

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

  2. 
      
André Klitzing
André Klitzing
Review request changed

Change Summary:

rebase to merged r9009

Commit:

-e68ca96568d04a3ad8a895de1294051ca065f32d
+16a5225e9f75426016dc79728fd9449c34337073

Diff:

Revision 5 (+743)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
André Klitzing
  1. 
      
  2. Do you accept it if I move rbtools/hook stuff to contrib/tools/mercurial_push.py ? :-)

  3. 
      
André Klitzing
  1. 
      
  2. 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

  3. 
      
Loading...