Buildbot plugin for ReviewBot

Review Request #3535 — Created Nov. 19, 2012 and submitted

Information

ReviewBot

Reviewers

This plugin will submit a patch to a buildmaster using the buildbot try command.

At the moment, PB authentication is the only working authentication method. SSH functionality is there, but buildbot itself is broken.
Only tested with git, but there's no reason why it shouldn't work with other SCM tools.

It successfully appends the result of a build to a review regardless of status.
Description From Last Updated

The api bindings were updated recently allowing the use of **kwargs instead of the 'values' argument. Can you change this …

SM smacleod

I know the pep8 tool isn't following the proper capitalization style, but can we switch this to be BuildbotTool.

SM smacleod

let's avoid the use of '\' here to break the lines. You can use the implicit string concatenation inside the …

SM smacleod

I don't think a default of True makes sense here.

SM smacleod

See above comment about style.

SM smacleod

The outside parens aren't aligned properly. I'd just shove the traliling paren up onto the line above.

SM smacleod

See above comment about style.

SM smacleod

Let's expand a bit and say something more specific about 'improperly stored'. e.g. The password will be stored as plaintext …

SM smacleod

See above about style

SM smacleod

See above about style.

SM smacleod

See above about style.

SM smacleod

Lets alphabetize this.

SM smacleod

Alphabetize this as well please.

SM smacleod

I don't think we need this newline.

mike_conleymike_conley

Should we report something instead of failing silently in this case? Is this case even possible?

mike_conleymike_conley

Nit - add doc

mike_conleymike_conley

For readability, are you able to indent this so that it's directly beneath the string that it continues? Same below …

mike_conleymike_conley

This blank line should be put back

SM smacleod

lets rename this diff_contents

SM smacleod

lets rename this get_diff_file_path

SM smacleod

Can we change this to something like: Runs buildbot try with the review requests diff.

SM smacleod

Alphabetize

SM smacleod

Alphabetical: from reviewbot.tools import Tool from reviewbot.tools.process import execute

SL slchen

Indentation issue here; I think you want: cmd = [ 'builtbot', 'try', ... ]

SL slchen

I'm not sure about this one, but I think it might be: branch = self.review.api_root.get_review_request( review_request_id=self.review.request_id ).branch Thoughts, mentors?

SL slchen

Indentation issue here: cmd.extend([ '--connect=pb', '--passwd=%s' % settings['password'], ])

SL slchen

Same comment as above.

SL slchen

Alphabetical

SL slchen
SM
  1. 
      
  2. bot/reviewbot/processing/review.py (Diff revision 1)
     
     
     
     
     
    The api bindings were updated recently allowing the use of **kwargs instead of the 'values' argument. Can you change this to:
    
    self.diff = api_root.get_diff(
        review_request_id=self.request_id,
        diff_revision=self.diff_revision).get_patch()
    
    I'd also like to change this so it isn't fetched for every execution. Some tools won't require this, and we don't want to make the request unless they need it.
  3. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
    I know the pep8 tool isn't following the proper capitalization style, but can we switch this to be BuildbotTool.
  4. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
     
    let's avoid the use of '\' here to break the lines.
    
    You can use the implicit string concatenation inside the dict, and we should also align the second line. Something like:
    
    'help_text': 'The address of the buildmaster. Used by both PB '
                 'and SSH',
  5. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
    I don't think a default of True makes sense here.
  6. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
     
    See above comment about style.
  7. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
     
     
    The outside parens aren't aligned properly. I'd just shove the traliling paren up onto the line above.
  8. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
     
     
    See above comment about style.
  9. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
    Let's expand a bit and say something more specific about 'improperly stored'.
    
    e.g. The password will be stored as plaintext in the database.
  10. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
     
    See above about style
  11. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
     
    See above about style.
  12. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
    This option could pose a problem if a number of workers require unique configurations... We might need to think up a different solution.
  13. bot/reviewbot/tools/buildbot.py (Diff revision 1)
     
     
     
    See above about style.
  14. bot/setup.py (Diff revision 1)
     
     
     
    Lets alphabetize this.
  15. bot/setup.py (Diff revision 1)
     
     
     
     
    Alphabetize this as well please.
    1. This is the name of the package for buildbot
    2. I know, I'd like this list of package dependencies alphabetized. (e.g. buildbot, celery, pep8)
  16. 
      
TA
mike_conley
  1. 
      
  2. bot/reviewbot/processing/review.py (Diff revision 2)
     
     
    I don't think we need this newline.
  3. bot/reviewbot/processing/review.py (Diff revision 2)
     
     
     
    Should we report something instead of failing silently in this case? Is this case even possible?
    1. I think so, I've been following the style of other methods within the review class which check to see if the api_root contains the right method and then returning None
      
      eg:
      if not hasattr(self._api_filediff, 'get_patched_file'):
          return None
    2. You don't actually need to be doing this check here. The case of 'get_patched_file'
      is special, since the patched_file link on a filediff resource won't exist if it's
      a deleted file. (original_file won't exist if the file is new).
      
      So just remove the check for get_diff.
  4. bot/reviewbot/processing/review.py (Diff revision 2)
     
     
    Nit - add doc
  5. bot/reviewbot/tools/buildbot.py (Diff revision 2)
     
     
    For readability, are you able to indent this so that it's directly beneath the string that it continues? Same below a few times.
  6. 
      
TA
SM
  1. 
      
  2. bot/reviewbot/processing/review.py (Diff revision 3)
     
     
    This blank line should be put back
    1. Whoops, I thought I had added that in
  3. bot/reviewbot/processing/review.py (Diff revision 3)
     
     
    lets rename this diff_contents
  4. bot/reviewbot/processing/review.py (Diff revision 3)
     
     
    lets rename this get_diff_file_path
  5. bot/reviewbot/tools/buildbot.py (Diff revision 3)
     
     
    Can we change this to something like:
    
    Runs buildbot try with the review requests diff.
  6. bot/setup.py (Diff revision 3)
     
     
     
    Alphabetize
  7. 
      
TA
TA
SL
  1. 
      
  2. bot/reviewbot/tools/buildbot.py (Diff revision 5)
     
     
     
    Alphabetical: 
    
    from reviewbot.tools import Tool
    from reviewbot.tools.process import execute
  3. bot/reviewbot/tools/buildbot.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
    Indentation issue here; I think you want:
    
    cmd = [
        'builtbot',
        'try',
        ...
    ]
  4. bot/reviewbot/tools/buildbot.py (Diff revision 5)
     
     
     
    I'm not sure about this one, but I think it might be: 
    
    branch = self.review.api_root.get_review_request(
        review_request_id=self.review.request_id
    ).branch
    
    Thoughts, mentors?
  5. bot/reviewbot/tools/buildbot.py (Diff revision 5)
     
     
     
     
    Indentation issue here:
    
    cmd.extend([
        '--connect=pb',
        '--passwd=%s' % settings['password'],
    ])
  6. bot/reviewbot/tools/buildbot.py (Diff revision 5)
     
     
     
     
     
     
    Same comment as above.
  7. bot/setup.py (Diff revision 5)
     
     
     
    Alphabetical
  8. 
      
TA
TA
TA
TA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to rework (8bf068132ae08cfbd9f787a812d977bb297afbdc)
Loading...