Fixed a bug where rbt land was unable to determine the approval state of a review request.

Review Request #8401 — Created Sept. 17, 2016 and submitted

Information

RBTools
release-0.7.x
49c4e26...

Reviewers

Fixed a bug where rbt land was unable to determine the approval state
of a review request.

This change introduces default values to is_rr_approved and
approval_failure before attempting to retrieve information from the
review request. In the event an exception is encountered, RBTools will
fail gracefully without disclosing a stack trace.

Ran the land command multiple times, and observed its functionality
with and without ship-its. Noted that functionality is unaffected
for normal use cases.

Description From Last Updated

Can you wrap your description and testing done at 72 characters?

brenniebrennie

You cleaned up the description but not the summary. Can you do that as well?

brenniebrennie

Nit: Needs period.

mike_conleymike_conley

Add a blank line after this.

daviddavid

So there are a couple states that can be hit where defaults come into play, not all of which are …

chipx86chipx86

Now that we have a generic exception handler I don't think we need to set the default values.

daviddavid

These are indented too far.

daviddavid

This should be raise CommandError(...)

brenniebrennie

Alphabetize these.

brenniebrennie

e on next line.

brenniebrennie
mike_conley
  1. This makes sense to me - only one small nit.

  2. rbtools/commands/land.py (Diff revision 1)
     
     

    Nit: Needs period.

  3. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
    
    
  2. 
      
RS
RS
david
  1. Can you fix up your summary to be a bit less specific about the code change? Something like "Fix a bug where rbt land was unable to find the approval state".

    In the description, it looks like your code blocks (things in `) have escape characters. Mind editing that so that any reference to is_rr_approved shows up with the nice code block syntax? Things like Exception and AttributeError should be similarly marked.

    1. Sounds good. I've updated the description / summary accordingly.

  2. rbtools/commands/land.py (Diff revision 2)
     
     

    Add a blank line after this.

  3. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
    
    
  2. 
      
brennie
  1. 
      
  2. Can you wrap your description and testing done at 72 characters?

  3. You cleaned up the description but not the summary. Can you do that as well?

  4. 
      
RS
chipx86
  1. Can you wrap your Description/Testing Done to about 75 characters per line? The text is used directly for commit messages, and having those at the proper length will help us land your changes sooner. Thanks!

    1. Sorry, I had this in a draft and failed to publish before when I made the below comment. I know you've already addressed this. Thanks!

    2. Cheers!

  2. rbtools/commands/land.py (Diff revision 3)
     
     
     
     
     
     

    So there are a couple states that can be hit where defaults come into play, not all of which are errors.

    1) It's an older Review Board, which doesn't have these fields (thus the AttributeError where we simulate it).
    2) We get an unknown exception (maybe review_request isn't what we think, or something else goes very wrong).
    3) This doesn't get hit in practice (but a linter would have a problem with it): is_rr_approved can be True while approval_failure can be undefined. This would happen if we hit the AttributeError and ship_it_count > 0. While not used directly, it would be nice to have some default here.

    The default added for approval_failure is only appropriate in case #2, where there's some unknown exception. Now, it's not clear from the bug report what sort of exception was hit, but I think what we should have here is an except Exception as e: in that whole try/except block.

    This handler wouldn't need to set a default, but rather would:

    1) Log the exception (using logging.exception and saying something like 'Unexpected error when looking up review request approval state: %s' % e)
    2) Immediately raise a CommandError with the text you're currently setting as the default for approval_failure.

    It might still make sense to set the defaults above, but I'd set approval_failure to None. This would just be to satisfy any linters, but it's too early to assume anything about the failure at this part of the code.

    1. Thanks for taking the time to write this all out.

      I see what you're saying about the possible states that can be hit here. I've updated my review request to incorporate the additional exception handling as suggested. I've left the original default approval_failure assignment in for now but changed it to None.

  3. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
    
    
  2. 
      
david
  1. 
      
  2. rbtools/commands/land.py (Diff revision 4)
     
     
     

    Now that we have a generic exception handler I don't think we need to set the default values.

  3. rbtools/commands/land.py (Diff revision 4)
     
     
     
     
     
     
     

    These are indented too far.

  4. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/land.py (Diff revision 5)
     
     

    This should be raise CommandError(...)

  3. 
      
RS
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/land.py (Diff revision 6)
     
     
     

    Alphabetize these.

  3. rbtools/commands/land.py (Diff revision 6)
     
     

    e on next line.

  4. 
      
RS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/land.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/land.py
    
    
  2. 
      
RS
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (51a7f7d)
Loading...