Bug 206532 – [ews] commit-queue should verify patch committer and reviewer

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

[Aakash Jain]

commit-queue should verify patch committer and reviewer from https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json and not land the patch in case committer and reviewer's don't have required privileges.

[Radar WebKit Bug Importer]

<rdar://problem/58758869>

[Aakash Jain]

Created attachment 391689 [details]
Patch

[Aakash Jain]

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

[Aakash Jain]

Created attachment 391692 [details]
Patch

[Jonathan Bedard]

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.

[Aakash Jain]

Created attachment 391751 [details]
Patch

[Aakash Jain]

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

[Jonathan Bedard]

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.

[Aakash Jain]

Created attachment 391755 [details]
Patch

[Aakash Jain]

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

[Jonathan Bedard]

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?

[Aakash Jain]

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

[Aakash Jain]

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

[Aakash Jain]

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