|Summary:||Refactor bugzilla-tool to allow for multiple queues|
|Product:||WebKit||Reporter:||Adam Barth <abarth>|
|Component:||Tools / Tests||Assignee:||Nobody <webkit-unassigned>|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
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 2 Darin Adler 2009-11-15 17:05:20 PST
Comment on attachment 43236 [details] Patch r=me 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] Patch 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. LIkewise: 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) 368382 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] Patch Ok. Let me try this again.
Comment 6 Eric Seidel (no email) 2009-11-15 23:56:37 PST
Comment on attachment 43270 [details] Arise WorkQueue safe_to_proceed_with_work_item 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. _ensure_bug_log_closed 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.