[webkit-reviews] review requested: [Bug 176526] Add support for builderNameToIDMap in BuildbotSyncer : [Attachment 326308] Updated patch

[bugzilla-daemon at webkit.org]

Aakash Jain <aakash_jain at apple.com> has asked  for review:
Bug 176526: Add support for builderNameToIDMap in BuildbotSyncer
https://bugs.webkit.org/show_bug.cgi?id=176526

Attachment 326308: Updated patch

https://bugs.webkit.org/attachment.cgi?id=326308&action=review




--- Comment #4 from Aakash Jain <aakash_jain at apple.com> ---
Created attachment 326308

  --> https://bugs.webkit.org/attachment.cgi?id=326308&action=review

Updated patch

Incorporated review comments.

> On my second thought, I don't think it makes much sense for BuildbotSyncer
> itself to have the map.
> Each syncer can be associated with exactly one builder so we should instead
> add object.builderID or something.

Done. This information is now in object.builderID.


> A more important code to have here is to check that each builder we find
appears in the mapping.
> In fact, since one of the most common bugs we find in our configuration is
> that some builders in config json are missing from buildbot, we should
validate that each builder name appears
> in the map.
> 
> We should actually implement this validation for buildbot 0.8 using
> /json/builders/ for now.

Implemented this validation, by fetching the builder list from buildbot 0.8
using /json/builders/ (for now) and asserting that the builder we found in
config is present in Buildbot mapping.