[Webkit-unassigned] [Bug 38918] add a FancyReviewQueue to upload r? patches to rietveld

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38918


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55908|review?                     |review-
               Flag|                            |




--- Comment #5 from Adam Barth <abarth at webkit.org>  2010-05-12 15:31:32 PST ---
(From update of attachment 55908)
Why do we support uploading directly to rietveld?  It seems like we want to remove that code in favor of always using the bot.

It's a little strange that we keep track of the "this patch is in rietveld" state twice: once in a bugzilla flag and again in AppEngine.  What if these get out of sync?  I think we only want to keep one copy of this information.

WebKitTools/Scripts/webkitpy/tool/commands/download.py:185
 +  # FIXME: Write a test for this code. Testing it is hard since it using Rietveld's upload.py,
Huh?  We should just mock that out.

WebKitTools/Scripts/webkitpy/tool/commands/download.py:188
 +      name = "post-attachment-fancy"
We need a better name...  send-attachment-to-rietveld ? post-code-review-for-attachment ?

WebKitTools/Scripts/webkitpy/tool/commands/download.py:190
 +      arguments_names = "[ATTACHMENTID]"
Why is this argument optional?

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:375
 +      name = "fancy-review-queue"
Again, we need a better name here.  This name is stored persistently on the server so we're stuck with it once we start.

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:385
 +          self.run_webkit_patch(["post-attachment-fancy", "--fancy-review", "--force-clean", "--non-interactive", "--parent-command=style-queue", patch.id()])
why is the style-queue the parent command?

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:386
 +          return True
Don't we need an error handler here?  What do you want to happen if we encounter an error?

WebKitTools/Scripts/webkitpy/tool/commands/upload.py: 
 +  
Why did you remove this line?

WebKitTools/Scripts/webkitpy/tool/mocktool.py:509
 +  class MockRietveld(Rietveld):
We don't have the mocks inherit from the real objects.  It's too dangerous.  The calls might leak through and do real things.

WebKitTools/Scripts/webkitpy/tool/steps/postcodereview.py:50
 +              self.tool.bugs.set_flag_on_attachment(patch.id, 'in-rietveld', '+', 'Patch has been upload to rietveld')
Where does this text go?  Is it spammed in email?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.