Bug 31513 – Refactor bugzilla-tool to allow for multiple queues

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Barth]

We should implement some infrastructure changes before settling on exactly what the other bots will do.

[Adam Barth]

Created attachment 43236 [details]
Patch

[Darin Adler]

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.

[Eric Seidel (no email)]

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

[Adam Barth]

Comment on attachment 43236 [details]
Patch

Ok.  Let me try this again.

[Adam Barth]

Created attachment 43270 [details]
Arise WorkQueue

[Eric Seidel (no email)]

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.

[Adam Barth]

	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/bugzilla-tool
Committed r51012