Bug 206532

<rdar://problem/58758869>

Created attachment 391689 [details] Patch

Sample runs: https://ews-build.webkit-uat.org/#/builders/26/builds/826 https://ews-build.webkit-uat.org/#/builders/26/builds/836 https://ews-build.webkit-uat.org/#/builders/26/builds/835

Created attachment 391692 [details] Patch

Comment on attachment 391692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391692&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:653 > + def load_contributors(self): This function is weird. At the moment, we only call it in the constructor and it sets up a cache, where it will return values. But that doesn't make sense because it's called in the constructor. Seems to me that we should either git rid of the early return and stop returning values since we don't actually use the returned values, or have this function only return the dictionary to the contributors which we then assign to the cache in the constructor.

Created attachment 391751 [details] Patch

(In reply to Jonathan Bedard from comment #5) > or have this function only return the dictionary of the contributors which we then assign to the cache in the constructor. Done in updated patch.

Comment on attachment 391751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391751&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:626 > + if not self.contributors: This new code made me realize that this data is cached at the class level, which I didn't notice before. First, I'm not sure we should cache it, we want to pick up when new contributors are added, right? And reading contributors shouldn't take too long, it seems fine to do it every time we construct the step. Second, we should declare load_contributors as a class method because that's what it is.

Created attachment 391755 [details] Patch

(In reply to Jonathan Bedard from comment #8) > First, I'm not sure we should cache it, we want to pick up when new contributors are added, right? Agree, updated in new patch. Also buildbot loads the __init__ method at the 'buildbot start' time, which we don't want. So moved the call to load load_contributors() inside start() and deleted __init__. Also I did some testing for network errors and improved error handling for the case when we are unable to load contributors.json. Tested in https://ews-build.webkit-uat.org/#/builders/26/builds/878 > Second, we should declare load_contributors as a class method because that's what it is. We are using self._addToLog inside this method to display the error message in UI. Need to keep it instance method for that.

Comment on attachment 391755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391755&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:632 > + return {} My only question here is: Should we used the cached version if we fail to load?

(In reply to Jonathan Bedard from comment #11) > My only question here is: Should we used the cached version if we fail to load? Let's start with this simple approach for now. Will add the caching logic later if needed.

Committed r257498: <https://trac.webkit.org/changeset/257498>

Seems to be working fine. e.g.: https://ews-build.webkit.org/#/builders/28/builds/99