Add Mercurial hook to post to ReviewBoard on push

Review Request #8554 — Created Nov. 26, 2016 and discarded

Information

RBTools
release-0.7.x

Reviewers

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

miserymisery

Col: 51 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

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

daviddavid

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

daviddavid

"Review Board"

daviddavid

"Review Board"

daviddavid

Typo: changset

daviddavid

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

daviddavid

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

daviddavid

This could be: return any( r.summary == request.summary for r in revreqs )

daviddavid

If this is similar to the methods in logging, you can pass the format argument in as an argument to …

daviddavid

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

daviddavid

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

daviddavid

Blank line between these two.

daviddavid

a reviewboard hook information?

daviddavid

Blank line after class docstrings.

daviddavid

I don't understand what this is supposed to be testing. It looks like it just processes a constant and checks …

daviddavid

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

daviddavid
reviewbot
  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)
     
     
    Show all issues
    Col: 51
     E226 missing whitespace around arithmetic operator
    
  3. 
      
misery
  1. 
      
  2. 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'
    
    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. 
      
misery
reviewbot
  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. 
      
misery
reviewbot
  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
  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)
     
     
    Show all issues

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

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

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

    "Review Board"

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

    "Review Board"

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

    Typo: changset

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

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

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

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

    This could be:

    return any(
        r.summary == request.summary
        for r in revreqs
    )
    
  10. 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.

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

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

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

    Blank line between these two.

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

    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)
     
     
    Show all issues

    Blank line after class docstrings.

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

    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)
     
     
    Show all issues

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

  18. 
      
misery
reviewbot
  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. 
      
misery
misery
misery
  1. 
      
  2. Do you accept it if I move rbtools/hook stuff to contrib/tools/mercurial_push.py ? :-)

  3. 
      
misery
  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. 
      
misery
Review request changed
Status:
Discarded
Change Summary:
https://github.com/misery/ExtendedApproval/blob/master/contrib/mercurial_git_push.py