Bug 38918 – add a FancyReviewQueue to upload r? patches to rietveld

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

add a FancyReviewQueue to upload r? patches to rietveld

[Ojan Vafai]

Created attachment 55724 [details]
Patch

[Adam Barth]

I thought we weren't calling it fancy review anymore?  (I don't seem to be able to cc jparent for some reason.)

[Ojan Vafai]

Created attachment 55908 [details]
Patch

[Ojan Vafai]

Comment on attachment 55908 [details]
Patch

I added support for the in-rietveld flag to this patch. It was easier to do this way. I can split it up if you need me to though.

[Adam Barth]

Comment on attachment 55908 [details]
Patch

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?

[Ojan Vafai]

Created attachment 55921 [details]
Patch

[Ojan Vafai]

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

[Ojan Vafai]

Created attachment 55928 [details]
Patch

[Ojan Vafai]

Comment on attachment 55928 [details]
Patch

Added a couple comments to get rid of --fancy-review and fixed a typo in postcodereview.py.

Also, now that the in-rietveld flag is active, you can see an example of what the post-attachment-to-rietveld command does on bug 26526.

[Eric Seidel (no email)]

The way the status bubbles work is to use an iframe to an appengine.  I think that would be cleaner than using an in-rietveld flag.

Flags cause bugzilla to send mail when they're changed, which is bad-news-bears.

[Ojan Vafai]

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

Actually, I take this back. I forgot that in-reitveld is a per-patch flag, but we only have one reitveld issue per bug. So we need in-rietveld to make it so we only show the rietveld review link for patches that have actually been posted to rietveld. Not sure how to get around this unless we kept a map of uploaded attachment IDs in rietveld, but that doesn't fix anything. It just changes where we store the bit, not that we store two bits. I'm open to suggestions.

I agree that the email sending is a problem. Is there anyway to get bugzilla not to send emails for some updates?

[Ojan Vafai]

Talked to Eric on IRC. Another option would be to have a separate server that just returns 200/404 if a given attachment has been uploaded to rietveld. This doesn't solve the storing the bit in multiple places problem, but it does solve the bugzilla sending spammy email problems.

I'd rather just solve the bugzilla problem though. Another server seems like more bits that need maintaining and can break.

Adam, we're not actually storing the same data twice currently. in-rietveld tells us which patches have been uploaded. Rietveld server only knows which bugs have rietveld issues, not which individual patches.

[Adam Barth]

> Adam, we're not actually storing the same data twice currently. in-rietveld tells us which patches have been uploaded. Rietveld server only knows which bugs have rietveld issues, not which individual patches.

The queue machinery is storing a bit per attachment to remember whether it has processed that attachment before (so that queue can be restarted / moved / etc).  That bit seems almost the same as the in-rietveld flag.

If you want to make bugzilla the authority for which patches have been processed, you want to model your bot after commit-queue, which uses the commit-queue flag to store that information.  Essentially, you want to move your bot up one layer in the class hierarchy so that it can manually manage the set of patches to process.

[Ojan Vafai]

Created attachment 56116 [details]
Patch

[Ojan Vafai]

Comment on attachment 56116 [details]
Patch

I made the queue more like the commit-queue and got rid of --fancy-review (so, upload/post no longer have an option to post to rietveld). upload/post just always set in-rietveld?, which then puts it in the queue to be uploaded to rietveld.

[Ojan Vafai]

Also, bug 39090 addressed the email spam issue.

[Ojan Vafai]

Created attachment 56123 [details]
Patch

[Ojan Vafai]

Comment on attachment 56123 [details]
Patch

Fixes a bug in rietveld.py that strips the first argv from RealMain. RealMain itself does that, so we were actually stripping the first two arguments, which mean that --assume-yes was getting dropped.

[Ojan Vafai]

Created attachment 56897 [details]
Patch

Sync to ToT

[Adam Barth]

Comment on attachment 56897 [details]
Patch

This is looking pretty good.  Some minor comments below.  Probably needs one more round.

WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:629
 +          self.browser['flag_type-4'] = ('?',)
It's too bad that this will only work for patches uploaded with webkit-patch.  What's the plan for getting all r? patches uploaded?  Why not start with that?

WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:805
 +              self.browser.set_value(comment_text, name='comment', nr=0)
I don't understand how this change relates...

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:336
 +          comment_text = "Rejecting patch %s from rietveld-queue." % patch.id()
Why might this happen?  Do we want to send email when it happens?

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:345
 +      def _error_message_for_bug(tool, status_id, script_error):
This function looks like a copy/paste of another function.  Can we share the implementation?

[Adam Barth]

@eric: Does this patch integrate properly with our new control channel / pending patches list?

[Ojan Vafai]

Created attachment 56910 [details]
Patch

[Ojan Vafai]

(In reply to comment #20)
> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:629
>  +          self.browser['flag_type-4'] = ('?',)
> It's too bad that this will only work for patches uploaded with webkit-patch.  What's the plan for getting all r? patches uploaded?  Why not start with that?

I was just trying to scope the change to avoid creating havoc if something goes wrong with the queue. But, whatever, the queue now processes r? and in-rietveld? matches.

> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:805
>  +              self.browser.set_value(comment_text, name='comment', nr=0)
> I don't understand how this change relates...

I had made the change in an earlier iteration of this patch and it seemed like a good one. Anyways, removed.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:336
>  +          comment_text = "Rejecting patch %s from rietveld-queue." % patch.id()
> Why might this happen?  Do we want to send email when it happens?

Only case I can think of is if rietveld is down. This will post to the bug, so no need to send a separate email, right?

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:345
>  +      def _error_message_for_bug(tool, status_id, script_error):
> This function looks like a copy/paste of another function.  Can we share the implementation?

Moved into StepSequenceErrorHandler. Not sure if that is the right place. I could create a AbstractPatchQueue/StepSequenceErrorHandler superclass, but that seems excessive. Putting in directly in AbstractPatchQueue seemed wrong since this used in an implementation of a StepSequenceErrorHandler method.

[Adam Barth]

Comment on attachment 56910 [details]
Patch

Minor nits below.  You should feel free to either address them before landing or in a follow-up patch.

WebKitTools/Scripts/webkitpy/common/net/rietveld.py:77
 +          issue, patchset = upload.RealMain(args, data=diff)
I don't quite understand this change, but ok.

WebKitTools/Scripts/webkitpy/tool/commands/queues.py:329
 +          comment_text = "Rejecting patch %s from rietveld-queue." % patch.id()
I still worry that this message is cryptic and folks won't understand it.  We can iterate after landing though.

WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py:39
 +      def _error_message_for_bug(tool, status_id, script_error):
This isn't quite the right place for this method.  StepSequenceErrorHandler is just an interface definition.  It shouldn't have any implementations.  We need to create an intermediate class that inherits from StepSequenceErrorHandler that the queues inherit from.

[Ojan Vafai]

(In reply to comment #24)
> (From update of attachment 56910 [details])
> Minor nits below.  You should feel free to either address them before landing or in a follow-up patch.
> 
> WebKitTools/Scripts/webkitpy/common/net/rietveld.py:77
>  +          issue, patchset = upload.RealMain(args, data=diff)
> I don't quite understand this change, but ok.

The original code was just dumb. It's stripping the first argument. But upload.py already does that. So we were doing it twice.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:329
>  +          comment_text = "Rejecting patch %s from rietveld-queue." % patch.id()
> I still worry that this message is cryptic and folks won't understand it.  We can iterate after landing though.

Tried to make the text more meaningful.

> WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py:39
>  +      def _error_message_for_bug(tool, status_id, script_error):
> This isn't quite the right place for this method.  StepSequenceErrorHandler is just an interface definition.  It shouldn't have any implementations.  We need to create an intermediate class that inherits from StepSequenceErrorHandler that the queues inherit from.

Added a FIXME for now. I was going to just do this, but I got stuck on testing it.

[Ojan Vafai]

Committed r60635: <http://trac.webkit.org/changeset/60635>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60635 might have broken GTK Linux 64-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60632
http://trac.webkit.org/changeset/60633
http://trac.webkit.org/changeset/60634
http://trac.webkit.org/changeset/60635
http://trac.webkit.org/changeset/60636