We are planning to upgrade the buildbot instance to 0.9 (latest version). Bot watcher dashboard would need update accordingly. Most importantly buildbot REST API through which dashboard fetches the data has changed significantly in buildbot 0.9 (as compared to buildbot 0.8). Dashboard code would need to be changed accordingly.
Also see <rdar://problem/33361327>.
Created attachment 316276 [details] Proposed patch
We would need some discussion about how to maintain compatibility with buildbot 0.8, during the transition period. We might consider forking the repository or adding a lot of if-else to make the code compatible with both buildbot 0.8 and 0.9.
I disagree with forking - I think we should determine a way to elegantly add support for both 0.8.6+ and 0.9.X, or upgrade OpenSource to 0.9.X as well.
Created attachment 316577 [details] Updated patch. Incorporated lot of Dan Bates' feedback from in-person review. Added support for 0.8 buildbot data. Tested the code with both buildbot 0.8 and 0.9.
Created attachment 316579 [details] Updated patch. Edited WebKitBuildbot.js to pass USE_BUILDBOT_VERSION_LESS_THAN_09 flag.
Created attachment 316580 [details] Dashboard-Screenshot Screenshot of OpenSource Dashboard after applying this patch attached.
Created attachment 316594 [details] Updated patch Incorporated Dan Bates feedback. Simplified the use of 'id' variable in BuildbotQueue.js. Earlier 'id' for each queue was being modified by Buildbot.js after slight delay (after doing a REST API call). Now, Buildbot.js does not modify the 'id' at all, the fetched information is only used inside Buildbot.js. Tested with buildbot 0.8 and 0.9.
Comment on attachment 316594 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=316594&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:269 > - this.resultURLs = masterShellCommandStep ? masterShellCommandStep.urls : null; > + if (masterShellCommandStep && masterShellCommandStep.urls) > + this.resultURLs = masterShellCommandStep.urls[0]; This is a change in behavior. How did you come to the decision to make this.resultURL a single URL as opposed to letting resultURLs be a list of URLs? Is it OK if this.resultURLs is undefined as opposed to null? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:324 > + data.steps[i].state_string = data.steps[i].results[1].join(' '); // e.g. "Exiting early after 20 crashes and 30 timeouts. 31603 tests run. 147 failures 69 new passes" I suggest we move the inline comment to be above this line since it is a long comment. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:325 > + data.steps[i].results = data.steps[i].results[0]; // See URL https://git.io/v7GO4 I suggest that we reference the Build Result Codes page in the Buildbot 0.9 documentation <http://docs.buildbot.net/0.9.9.post2/developer/results.html#module-buildbot.process.results> instead of linking to the code because the Build Result Codes both lists the different result codes and explains each one. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:330 > + masterShellCommandStep.urls = [masterShellCommandStep.urls]; I would have expected that a property named "urls" (plural for URL) would be an array that contains one or more URLs. What is the data type of masterShellCommandStep.urls? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:411 > + }.bind(this), > + this.urlFailedToLoad, > + {withCredentials: this.queue.buildbot.needsAuthentication}); I do not see the need to write this across three lines. I would write this all on one-line. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:418 > + this._buildData = data.builds[0]; How did you come to pick the first build? Or is data.builds a misnomer and it should be renamed data.build that represents a single build? > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:422 > + }.bind(this), > + this.urlFailedToLoad, > + {withCredentials: this.queue.buildbot.needsAuthentication}); Ditto.
Created attachment 316686 [details] Updated patch Incorporated the review comments. > How did you come to pick the first build? Or is data.builds a misnomer and it should be renamed data.build that represents a single build? Yes, data.builds represents a single build here (since we fetched info for only one specific build in this class). Buildbot has this weird way of referring everything as a plural, even though it is singular. See: https://docs.buildbot.net/0.9.0/developer/rest.html#collections
Comment on attachment 316686 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=316686&action=review This patch looks good. We could take this opportunity to modernize the code we are modifying using ES6 enchantments. This is not strictly necessary to do in this patch. We may want to consider instantiating and retuning new objects from _adjustBuildDataIfNeeded() instead of mutating the existing one as mutating an existing object can be error prone. > Tools/ChangeLog:3 > + Update Bot Watcher's Dashboard for buildbot 0.9 buildbot => Buildbot (Please fix all such occurrences. There are many occurrences throughout this ChangeLog entry.) > Tools/ChangeLog:10 > + (Buildbot.prototype._computeBuilderNameToIDMap): Fetch the builder name to ID mapping from buidbot and store buidbot => Buildbot > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js:172 > + // FIXME: Remove buildbotNineIDMap lookup after <https://github.com/buildbot/buildbot/issues/3465> is fixed. buildbotNineIDMap => this._builderNameToIDMap > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:100 > - return property[0] === "got_revision" && typeof property[1] === "object"; > + return typeof property[0] === "object"; This makes the code a bit more more error prone so long as we still support Buildbot less than version 0.9. For your consideration, I suggest that we have this function take whether we are using Buildbot 0.9 or less and do what we do now if it is less than 0.9. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:113 > - // ["got_revision",{"Internal":"1357","WebKitOpenSource":"2468"},"Source"] > + // [{"Internal":"1357","WebKitOpenSource":"2468"},"Source"] > // OR > - // ["got_revision","2468","Source"] > + // ["2468","Source"] We haven't made the switch to Buildbot 0.9 yet. So, we should not change this comment to reflect the Buildbot 0.9 format until then. For now, I suggest we update this comment to document both formats. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:114 > if (isMultiCodebaseGotRevisionProperty(property)) I would pass this.queue.buildbot.VERSION_LESS_THAN_09 to this function. See my remark in isMultiCodebaseGotRevisionProperty() for more details. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:246 > + else if (data.sourceStamps) We may want to add a console.assert() above this block to ensure either data.sourceStamp or data.sourceStamps is defined when using Buildbot version less than 0.9. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:276 > + /* Sample masterShellCommandStep.urls data: > + * "urls": [ > + * { > + * "name": "view results", > + * "url": "/results/Apple Sierra Release WK2 (Tests)/r220013 (3245)/results.html" > + * } > + * ] > + */ Please use C++-style comments (starts with //). > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:312 > + _adjustBuildDataIfNeeded: function(data) Maybe a better name for this function would be _adjustBuildDataForBuildbot09. For your consideration I suggest that we add a FIXME comment above this function to remove it once we switch to Buildbot 0.9 or later. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:318 > + data.started_at = data.times[0]; > + data.complete_at = data.times[1]; We should delete property times to prevent a programming mistake. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:322 > + var revisionProperty = data.properties.findFirst(function(property) { > + return property[0] === "got_revision"; > + }); This is OK as-is. We could modernize this by using let and a ES6 arrow function: let revisionProperty = data.properties.findFirst((property) => property[0] === "got_revision"); > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:328 > + revisionProperty.splice(0, 1); We should console.assert(revisionProperty[0] === "got_revision"). > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:332 > + for (var i = 0; i < data.steps.length; i++) { We can simplify this code and remove the need for an index by using an ES6 for-of loop: for (let step of data.steps) { ... } > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:333 > + data.steps[i].complete = data.steps[i].isFinished; We should delete isFinished from the step to prevent a programming mistake. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:336 > + data.steps[i].results = data.steps[i].results[0]; // See URL http://docs.buildbot.net/latest/developer/results.html Nit: There should only be one spec between the ';' and the start of the comment. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:339 > + var masterShellCommandStep = data.steps.findFirst(function(step) { return step.name === "MasterShellCommand"; }); This is OK as-is. We could write this using let and an ES6 arrow function: let masterShellCommandStep = data.steps.findFirst((step) => step.name === "MasterShellCommand"); > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:343 > + Minor: I do not see the need for this empty line. One empty line is sufficient to demarcate the block above from the block below. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:344 > + data.state_string = data.text.join(" "); We should delete the property text to prevent a programming mistake, > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:437 > + /* Sample data for a single build: > + * "builds": [ > + * { > + * "builderid": 282, > + * "buildid": 5609, > + * "complete": true, > + * ... > + * "workerid": 188 > + * } > + * ] > + */ Please use C++ style comments (that start with //). > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:245 > + return (data.builds[index].number); The parentheses are not needed.
Comment on attachment 316686 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=316686&action=review Thanks for the review. I have incorporated most of the feedback. Few comments below. > buildbot => Buildbot > > (Please fix all such occurrences. There are many occurrences throughout this ChangeLog entry.) Done. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:100 >> + return typeof property[0] === "object"; > > This makes the code a bit more more error prone so long as we still support Buildbot less than version 0.9. For your consideration, I suggest that we have this function take whether we are using Buildbot 0.9 or less and do what we do now if it is less than 0.9. This data is already modified by _adjustBuildDataForBuildbot09. The data at this point is always in Buildbot 0.9 format. > We haven't made the switch to Buildbot 0.9 yet. So, we should not change this comment to reflect the Buildbot 0.9 format until then. For now, I suggest we update this comment to document both formats. Done. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:246 >> + else if (data.sourceStamps) > We may want to add a console.assert() above this block to ensure either data.sourceStamp or data.sourceStamps is defined when using Buildbot version less than 0.9. Done. > Please use C++-style comments (starts with //). Done. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:312 >> + _adjustBuildDataIfNeeded: function(data) > > Maybe a better name for this function would be _adjustBuildDataForBuildbot09. For your consideration I suggest that we add a FIXME comment above this function to remove it once we switch to Buildbot 0.9 or later. Done. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:318 >> + data.complete_at = data.times[1]; > We should delete property times to prevent a programming mistake. Done. > This is OK as-is. We could modernize this by using let and a ES6 arrow function: > > let revisionProperty = data.properties.findFirst((property) => property[0] === "got_revision"); Done. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:328 >> + revisionProperty.splice(0, 1); > We should console.assert(revisionProperty[0] === "got_revision"). Added. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:332 >> + for (var i = 0; i < data.steps.length; i++) { > > We can simplify this code and remove the need for an index by using an ES6 for-of loop: > > for (let step of data.steps) { > ... > } Since we need to modify the array elements (modify the value of the steps), for-of loop might not work off the shelf. We can make it work, but it won't be significant simplification. Let's keep this loop for now. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:333 >> + data.steps[i].complete = data.steps[i].isFinished; > We should delete isFinished from the step to prevent a programming mistake. Done. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:339 >> + var masterShellCommandStep = data.steps.findFirst(function(step) { return step.name === "MasterShellCommand"; }); > > This is OK as-is. We could write this using let and an ES6 arrow function: > > let masterShellCommandStep = data.steps.findFirst((step) => step.name === "MasterShellCommand"); Done. > We should delete the property text to prevent a programming mistake, Done. > Please use C++ style comments (that start with //). Done. > The parentheses are not needed. Removed.
Created attachment 316909 [details] Updated patch for landing
Comment on attachment 316909 [details] Updated patch for landing Clearing flags on attachment: 316909 Committed r220120: <http://trac.webkit.org/changeset/220120>
All reviewed patches have been landed. Closing bug.