Bug 31513 - Refactor bugzilla-tool to allow for multiple queues
Summary: Refactor bugzilla-tool to allow for multiple queues
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 31422
  Show dependency treegraph
Reported: 2009-11-14 18:07 PST by Adam Barth
Modified: 2009-11-16 00:08 PST (History)
1 user (show)

See Also:

Patch (14.65 KB, patch)
2009-11-14 18:09 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Arise WorkQueue (14.69 KB, patch)
2009-11-15 23:42 PST, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-11-14 18:07:34 PST
We should implement some infrastructure changes before settling on exactly what the other bots will do.
Comment 1 Adam Barth 2009-11-14 18:09:07 PST
Created attachment 43236 [details]
Comment 2 Darin Adler 2009-11-15 17:05:20 PST
Comment on attachment 43236 [details]


While I'm not enough of a Python expert to spot every last thing, this all looks right to me. Please let me know if you'd prefer that someone more engaged with the process of making the new tools review patches like this one.
Comment 3 Eric Seidel (no email) 2009-11-15 18:35:59 PST
Comment on attachment 43236 [details]

I'm confused by this:
 350                 if options.try_queue:
 351                     pass # FIXME: Implement me!
 352                 else:
 353                     comment_text = WebKitLandingScripts.build_test_and_commit(tool.scm(), options)

It seems that that should just be factored out into its own method which is not call for hte try-queue.

 365                 tool.bugs.reject_patch_from_commit_queue(patch['id'], e.message_with_output())
 366             elif options.try_queue:
 367                 pass # FIXME: Implement me!

Why do you have:
809     def reject_patch(self, patch_id, message):
 810         return tool.bugs.reject_patch_from_commit_queue(patch_id, message)

If you're not using it?

Again here?
364376             if options.commit_queue:
365377                 patches = tool.bugs.fetch_commit_queue_patches_from_bug(bug_id, reject_invalid_patches=True)
 378             elif options.try_queue:
 379                 patches = [] # FIXME: Implement me!
366380             else:
367381                 patches = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)

Seems like you had one design idea, then you changed your mind, and then never went back to clean up the first idea...
Comment 4 Adam Barth 2009-11-15 20:49:32 PST
Comment on attachment 43236 [details]

Ok.  Let me try this again.
Comment 5 Adam Barth 2009-11-15 23:42:01 PST
Created attachment 43270 [details]
Arise WorkQueue
Comment 6 Eric Seidel (no email) 2009-11-15 23:56:37 PST
Comment on attachment 43270 [details]
Arise WorkQueue

maybe "should_proceed_with_work_item" would be better.

 862         # We don't have a patch id at this point, so try to grab the first patch off of the bug in question.

Would be better to update that comment to indicate that we plan to update the commit-queue to opearate off of patch ids in the near future.

819             make_option("--status-host", action="store", type="string", dest="status_host", default=StatusBot.default_host, help="Do not ask the user for confirmation before running the queue.  Dangerous!"),
should probably move up into WorkQueue since it's used there.


Seems like the wrong name for a generic queue.

_ensure_work_item_log_closed ?

This should eventually move to a new file.  It also eventually needs unit tests.

I trust you to make those edits before commit.
Comment 7 Adam Barth 2009-11-16 00:08:29 PST
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/bugzilla-tool
Committed r51012