[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





--- Comment #7 from Ojan Vafai <ojan at chromium.org>  2010-05-12 18:19:17 PST ---
(In reply to comment #5)
> Why do we support uploading directly to rietveld?  It seems like we want to remove that code in favor of always using the bot.

I agree. I was just thinking that would be a followup patch.

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

I agree it's a little strange, but we use this information to decide whether or not to show the the rietveld review UI in bugzilla. We considered having bugzilla ping appengine to see if the review existed, but that seemed janktastic. Ultimately, the bot will be running and uploading every patch to rietveld, so this flag will be obsoleted and we can get rid of it entirely. It's just an interim state to avoid showing the rietveld review UI on all patches until we can rely on the bot.

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

Ugh. Sorry. I forgot to go back and do this. As you can see, I had already mocked it out elsewhere. Anyways, fixed.

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

post-attachment-to-rietveld?

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

Fixed.

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

rietveld-upload-queue?

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

Copy-paste error.

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

Done.

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

Accidental.

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

Make sense.

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

I made it exclude the comment text.

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