Add a feeder queue that polls bugs.webkit.org for the commit-cluster
Created attachment 68161 [details] wip
Created attachment 68165 [details] work in progress
Created attachment 68171 [details] Patch
Attachment 68171 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:38: deprecated form of raising exception [pep8/W602] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 68171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68171&action=review In general looks great! Please address the nits above. > WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:35 > + self.tool = tool Why not _tool? :) > WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:55 > + self.update_work_items([patch.id() for patch in patches]) I might have put this out into a patch_ids local (I know it wasn't before). Doesn't really matter. > WebKitTools/Scripts/webkitpy/tool/bot/feeders.py:60 > + all_patches = sum([self.tool.bugs.fetch_bug(bug_id).commit_queued_patches(include_invalid=True) for bug_id in bug_ids], []) I might have split this out into a helper function to make this line shorter were I to write this again. (Again, I know this is my fault, just noticing as I re-read the code): [self._cq_patches_for_bug(bug_id) for bug_id in bug_ids]? donno. Again, doesn't really matter. > WebKitTools/Scripts/webkitpy/tool/commands/queues.py:149 > +class FeederQueue(AbstractQueue): I think you should add a comment above which explains why this makes sense as an AbstractQueue. It's not immediately obvious, given that it's such a strange queue. > WebKitTools/Scripts/webkitpy/tool/commands/queues.py:160 > + self.feeders = [ > + CommitQueueFeeder(self.tool), > + ] This is nice. I wonder if it should be a list of classes instead of instances. I seem to remember having the same debate with myself for Commands. > WebKitTools/Scripts/webkitpy/tool/commands/queues.py:163 > + return "synthetic-work-item" I would have added a comment here. > WebKitTools/Scripts/webkitpy/tool/commands/queues.py:166 > + return True Seems this should just be the default. Silly to make subclasses implement this. > WebKitTools/Scripts/webkitpy/tool/commands/queues.py:169 > + self._update() _update_checkout() maybe? > WebKitTools/Scripts/webkitpy/tool/commands/queues.py:173 > + for feeder in self.feeders: > + feeder.feed() > + time.sleep(self._sleep_duration) > + return True So gorgeous. :) > WebKitTools/Scripts/webkitpy/tool/commands/queues.py:176 > + def work_item_log_path(self, work_item): > + return os.path.join(self._log_directory(), "feeder.log") This is a strange case. We'll end up loggin the same thing twice. Maybe this should instead understand returning None and then not opening a work log? > WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py:113 > + queue = FeederQueue() > + queue._sleep_duration = 0 This should probably be a FeederQueue subclass since many tests are going to want this same behavior, no? > WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py:197 > + "next_work_item": "", Are these empty ones really required? I can't remember. > WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py:104 > + if isinstance(queue, StepSequenceErrorHandler): Seems we should add a comment about future refactoring here.
Committed r67913: <http://trac.webkit.org/changeset/67913>