Add mercurial hook for posting review requests automatically

Review Request #7824 — Created Dec. 23, 2015 and discarded

misery
RBTools
master
7823
rbtools
halvorlu
A Mercurial hook to post to Review Board on push to a central server.

The hook only allows the 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. makes links of all 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.

Patch is based on work of Halvor Lund!

Replacing this: https://reviews.reviewboard.org/r/7100/

We use this commit hook since february 2015 as a gatekeeper like gerrit codereview.

  • 8
  • 0
  • 44
  • 0
  • 52
Description From Last Updated
This should be grouped with the other stdlib imports. david david
Call this "logger" (since it's not really a constant) david david
Put this import at the top of the file. david david
Rather than implementing a new execute method, we should use rbtools.utils.process.execute david david
This should use the approval API. See https://www.reviewboard.org/docs/manual/2.5/webapi/2.0/resources/review-request/#fields david david
Can you wrap the whole conditional in parens instead of using the continuation character? Please also put the "and" on ... david david
Why doesn't this use rbtools.utils.users.get_authenticated_session ? david david
Put this import at the top of the file. david david
david
  1. 
      
  2. contrib/tools/mercurial_push.py (Diff revision 1)
     
     

    We've been slowly transitioning to Google Python-style docstrings. Can you write this as:

    Run the hook.
    
    Args:
        node (unicode): The hex node ID of the first changeset.
    
  3. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
     
     

    Can you use the Args: format as described above?

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

    Can we wrap this so each condition is on its own line? You can also combine the second two:

    if (revreq.branch == branch and
        'diff_hash' in revreq.extra_data.get('diff_hash', '')):
    
    1. Are you sure? I changed it to this:

      if (revreq.branch == branch and
          diff_hash == revreq.extra_data.get('diff_hash'):
      
    2. You're right. I was confusing this with other code I was reading in another tab (that used in).

  5. contrib/tools/mercurial_push.py (Diff revision 1)
     
     
     
     
     
  6. rbtools/hooks/common.py (Diff revision 1)
     
     
     

    r before s.

  7. rbtools/hooks/common.py (Diff revision 1)
     
     

    ids is already a list here.

  8. rbtools/hooks/mercurial.py (Diff revision 1)
     
     
     
     
     
     

    We put full-package imports before from ... imports.

  9. rbtools/hooks/mercurial.py (Diff revision 1)
     
     
     

    Process return codes should be int, not bool.

  10. rbtools/hooks/mercurial.py (Diff revision 1)
     
     
     
     

    Please reformat as:

    revisions = {
        'base': rev1,
        'tip': rev2,
        'parent_base': parent,
    }
    
  11. 
      
misery
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    WARNING: Number of comments exceeded maximum, showing 30 of 69.
    
    Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.py
        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 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  3. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 9
     E128 continuation line under-indented for visual indent
    
  4. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  5. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 23
     E251 unexpected spaces around keyword / parameter equals
    
  7. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 25
     E251 unexpected spaces around keyword / parameter equals
    
  8. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 5
     E128 continuation line under-indented for visual indent
    
  9. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 5
     E122 continuation line missing indentation or outdented
    
  10. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 22
     E251 unexpected spaces around keyword / parameter equals
    
  11. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 24
     E251 unexpected spaces around keyword / parameter equals
    
  12. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 5
     E122 continuation line missing indentation or outdented
    
  13. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 1
     E122 continuation line missing indentation or outdented
    
  14. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 15
     E251 unexpected spaces around keyword / parameter equals
    
  15. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  16. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  17. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 22
     E251 unexpected spaces around keyword / parameter equals
    
  18. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 15
     E251 unexpected spaces around keyword / parameter equals
    
  19. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  20. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 11
     E251 unexpected spaces around keyword / parameter equals
    
  21. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 13
     E251 unexpected spaces around keyword / parameter equals
    
  22. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 21
     E251 unexpected spaces around keyword / parameter equals
    
  23. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 23
     E251 unexpected spaces around keyword / parameter equals
    
  24. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 15
     E251 unexpected spaces around keyword / parameter equals
    
  25. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 17
     E251 unexpected spaces around keyword / parameter equals
    
  26. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 26
     E251 unexpected spaces around keyword / parameter equals
    
  27. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 28
     E251 unexpected spaces around keyword / parameter equals
    
  28. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 18
     E251 unexpected spaces around keyword / parameter equals
    
  29. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 20
     E251 unexpected spaces around keyword / parameter equals
    
  30. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 19
     E251 unexpected spaces around keyword / parameter equals
    
  31. contrib/tools/mercurial_push.py (Diff revision 2)
     
     
    Col: 21
     E251 unexpected spaces around keyword / parameter equals
    
  32. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
  1. Thanks for taking the time to submit this again. What are the main differences from my original submission?

    1. I just fixed the issues in your review request. :-)
      I added a new request because I cannot update your request. I will fix these issues here, too. Hopefully we get this merged!

  2. 
      
misery
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.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/hooks/common.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/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. rbtools/hooks/mercurial.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.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/hooks/common.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/hooks/common.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/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
  1. Ship It!
  2. 
      
david
  1. 
      
  2. contrib/tools/mercurial_push.py (Diff revision 6)
     
     

    It's a little weird to call this LOGGER, since it's not a constant. How about logger?

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

    Can you surround the whole conditional in parentheses instead of using the continuation character?

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

    These should be at the top of the file.

  5. rbtools/hooks/mercurial.py (Diff revision 6)
     
     
     
     
     

    Can we swap the order of these?

  6. 
      
misery
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/hooks/common.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/hooks/common.py
        rbtools/hooks/mercurial.py
        contrib/tools/mercurial_push.py
        rbtools/clients/mercurial.py
        rbtools/hooks/tests.py
    
    
  2. 
      
david
  1. Sorry for the long delay on this.

  2. rbtools/hooks/mercurial.py (Diff revision 7)
     
     

    This should be grouped with the other stdlib imports.

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

    Call this "logger" (since it's not really a constant)

  4. rbtools/hooks/mercurial.py (Diff revision 7)
     
     

    Put this import at the top of the file.

  5. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
     
     
     
     
     
     
     

    Rather than implementing a new execute method, we should use rbtools.utils.process.execute

  6. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
     
     

    This should use the approval API. See https://www.reviewboard.org/docs/manual/2.5/webapi/2.0/resources/review-request/#fields

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

    Can you wrap the whole conditional in parens instead of using the continuation character? Please also put the "and" on the previous line.

  8. rbtools/hooks/mercurial.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Why doesn't this use rbtools.utils.users.get_authenticated_session ?

  9. rbtools/hooks/mercurial.py (Diff revision 7)
     
     

    Put this import at the top of the file.

  10. 
      
misery
Review request changed

Status: Discarded

Change Summary:

This is superseded by a complete refactoring: https://reviews.reviewboard.org/r/8554/

Loading...