Bug 235074 - [EWS] Load contributors from stand-alone class
Summary: [EWS] Load contributors from stand-alone class
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-11 10:12 PST by Jonathan Bedard
Modified: 2022-01-19 10:08 PST (History)
4 users (show)

See Also:


Attachments
Patch (10.36 KB, patch)
2022-01-11 10:40 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.43 KB, patch)
2022-01-11 14:52 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.56 KB, patch)
2022-01-12 08:16 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2022-01-12 08:25 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (6.20 KB, patch)
2022-01-12 10:48 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (6.77 KB, patch)
2022-01-12 11:06 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.88 KB, patch)
2022-01-12 11:24 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.98 KB, patch)
2022-01-12 11:32 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.99 KB, patch)
2022-01-12 11:57 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.01 KB, patch)
2022-01-12 12:10 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.10 KB, patch)
2022-01-12 13:23 PST, Jonathan Bedard
darin: review+
jbedard: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2022-01-11 10:12:19 PST
The hook that GitHub sends buildbot has all the information we need, we should parse it's output into build properties.
Comment 1 Radar WebKit Bug Importer 2022-01-11 10:12:34 PST
<rdar://problem/87406157>
Comment 2 Jonathan Bedard 2022-01-11 10:16:43 PST
Pull-request: https://github.com/WebKit/WebKit/pull/69
Comment 3 Jonathan Bedard 2022-01-11 10:40:32 PST
Created attachment 448854 [details]
Patch
Comment 4 Jonathan Bedard 2022-01-11 14:52:52 PST
Created attachment 448882 [details]
Patch
Comment 5 Aakash Jain 2022-01-12 05:33:38 PST
Comment on attachment 448882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448882&action=review

> Tools/ChangeLog:3
> +        [EWS] Set build properties based on hook

based on hook => based on GitHub Pull Request hooks

> Tools/CISupport/ews-build/events.py:224
> +        properties['title'] = payload.get('title', '?')

can you provide example of build with and without this change?

> Tools/CISupport/ews-build/events.py:231
> +        contributors, _ = Contributors.load()

Note that Contributors.load() would be a network call to github.com. Please ensure that it is intentional that you want to do network call.

> Tools/CISupport/ews-build/steps.py:59
> +GITHUB_PROJECTS = ['WebKit/WebKit']

do we intend to add more projects here in future?

> Tools/CISupport/ews-build/steps.py:81
> +    def pr_url(cls, repository, pr_number):

can/should we make repository as optional parameter defaulting to 'WebKit/WebKit'?
Also repository => repository_url

> Tools/CISupport/ews-build/steps.py:84
> +        if not pr_number or isinstance(1234, int):

1234 seems to be from debugging, please update accordingly.

> Tools/CISupport/ews-build/steps.py:96
> +        repo_root = os.path.dirname(os.path.dirname(os.path.dirname(cwd)))

curious if this is correct or need one more os.path.dirname

> Tools/CISupport/ews-build/steps.py:116
> +        lines = []

lines/line should be replaced with errors/error through-out this patch, the generic variable name 'lines' is making the code logic harder to follow. At one point in this review, when I saw 'lines' variable being printed to the stdio, I thought it was just for debugging and left by mistake.

> Tools/CISupport/ews-build/steps.py:130
> +            github = value.get('github')

github => github_username

> Tools/CISupport/ews-build/steps.py:135
> +                contributors[github] = '{} <{}>'.format(name, emails[0].lower())

instead of string, we might want to store this as a dictionary (containing name, status and maybe email), e.g.: we might want to validate that the given github username is a reviewer or not.

> Tools/CISupport/ews-build/steps.py:192
> +            self.addURL('Pull request {}'.format(pr_number), GitHub.pr_url(self.getProperty('repository'), pr_number) or '?')

Please check if we can skip passing repository property here instead either use default value of read the property inside the pr_url method.
Also '?' => '' so that the link isn't clickable.

> Tools/CISupport/ews-build/steps.py:1009
> +    def __init__(self, *args, **kwargs):

Do we need this init method?

> Tools/CISupport/ews-build/steps.py:1054
> +        for line in lines:

Ditto. Please rename to: for error in errors: 
otherwise it's very confusing what is being printed here. Separately we can also do a print statement here so that it is also printed to server logs (in case we want to search the server logs)
Comment 6 Aakash Jain 2022-01-12 05:37:15 PST
Overall I feel like there should be a better way to deal with these properties. If buildbot doesn't provide these properties, we should change buildbot code to parse these out as this can be useful for everyone. But that upstream process can be independent of this patch.

Let me also look into buildbot code to figure out if there is a better way to achieve this.
Comment 7 Jonathan Bedard 2022-01-12 07:38:31 PST
Comment on attachment 448882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448882&action=review

>> Tools/CISupport/ews-build/steps.py:59
>> +GITHUB_PROJECTS = ['WebKit/WebKit']
> 
> do we intend to add more projects here in future?

Yes, for security bug support (which would be a different project, likely WebKit/WebKit-Security or something to that effect)

>> Tools/CISupport/ews-build/steps.py:81
>> +    def pr_url(cls, repository, pr_number):
> 
> can/should we make repository as optional parameter defaulting to 'WebKit/WebKit'?
> Also repository => repository_url

repository will be https://github.com/WebKit/WebKit. And a default could make sense here.

>> Tools/CISupport/ews-build/steps.py:192
>> +            self.addURL('Pull request {}'.format(pr_number), GitHub.pr_url(self.getProperty('repository'), pr_number) or '?')
> 
> Please check if we can skip passing repository property here instead either use default value of read the property inside the pr_url method.
> Also '?' => '' so that the link isn't clickable.

We can't skip passing repository here because we will have different repositories for security bugs in the future.

>> Tools/CISupport/ews-build/steps.py:1009
>> +    def __init__(self, *args, **kwargs):
> 
> Do we need this init method?

I'm not very familiar with Buildbot's object lifecycle. I assumed we wanted to guarantee self.contributors is set, is this the best way to do that?
Comment 8 Jonathan Bedard 2022-01-12 07:51:09 PST
Comment on attachment 448882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448882&action=review

>> Tools/CISupport/ews-build/events.py:231
>> +        contributors, _ = Contributors.load()
> 
> Note that Contributors.load() would be a network call to github.com. Please ensure that it is intentional that you want to do network call.

Got my wires a bit crossed here. I thought it was default disk load and fallback network load. But it's actually default network load and default disk load. Going to do a bit of caching here.

>> Tools/CISupport/ews-build/steps.py:96
>> +        repo_root = os.path.dirname(os.path.dirname(os.path.dirname(cwd)))
> 
> curious if this is correct or need one more os.path.dirname

Copied this from existing code, I think it's right?

dirname('<repo>/Tools/CISupport/ews-build/steps.py') -> '<repo>/Tools/CISupport/ews-build'
dirname('<repo>/Tools/CISupport/ews-build') -> '<repo>/Tools/CISupport'
dirname('<repo>/Tools/CISupport') -> '<repo>/Tools'
dirname('<repo>/Tools') -> '<repo>'
Comment 9 Jonathan Bedard 2022-01-12 08:16:39 PST
Created attachment 448947 [details]
Patch
Comment 10 Jonathan Bedard 2022-01-12 08:25:37 PST
Created attachment 448948 [details]
Patch
Comment 11 Aakash Jain 2022-01-12 08:31:16 PST
(In reply to Aakash Jain from comment #6)
> Let me also look into buildbot code to figure out if there is a better way to achieve this.

github_property_whitelist might achieve this functionality of parsing build properties from the hook, see: https://docs.buildbot.net/latest/manual/configuration/wwwhooks.html#github-hook
"A list of fnmatch expressions which match against the flattened pull request information JSON"
Comment 12 Jonathan Bedard 2022-01-12 08:36:21 PST
To Aakash's previous ask:
With the change: https://ews-build.webkit-uat.org/#/builders/9/builds/1963
Without the change: https://ews-build.webkit-uat.org/#/builders/9/builds/1964
Comment 13 Jonathan Bedard 2022-01-12 08:39:52 PST
(In reply to Aakash Jain from comment #11)
> (In reply to Aakash Jain from comment #6)
> > Let me also look into buildbot code to figure out if there is a better way to achieve this.
> 
> github_property_whitelist might achieve this functionality of parsing build
> properties from the hook, see:
> https://docs.buildbot.net/latest/manual/configuration/wwwhooks.html#github-
> hook
> "A list of fnmatch expressions which match against the flattened pull
> request information JSON"

This does work, https://ews-build.webkit-uat.org/#/builders/9/builds/1965, although I'm not sure that it's more clear than us setting the properties ourselves. And we can't convert GitHub usernames to name + email.
Comment 14 Jonathan Bedard 2022-01-12 10:47:44 PST
Aakash and I discussed achieving the original goals of this change in https://github.com/WebKit/WebKit/pull/66. Re-purposing to cover just the contributors.json change.
Comment 15 Jonathan Bedard 2022-01-12 10:48:44 PST
Created attachment 448963 [details]
Patch
Comment 16 Jonathan Bedard 2022-01-12 11:06:21 PST
Created attachment 448968 [details]
Patch
Comment 17 Jonathan Bedard 2022-01-12 11:24:08 PST
Created attachment 448974 [details]
Patch
Comment 18 Jonathan Bedard 2022-01-12 11:32:13 PST
Created attachment 448975 [details]
Patch
Comment 19 Jonathan Bedard 2022-01-12 11:57:15 PST
Created attachment 448981 [details]
Patch
Comment 20 Jonathan Bedard 2022-01-12 12:10:34 PST
Created attachment 448983 [details]
Patch
Comment 21 Jonathan Bedard 2022-01-12 13:23:08 PST
Created attachment 448990 [details]
Patch
Comment 22 Jonathan Bedard 2022-01-12 15:37:41 PST
Returning this bug to it's old name and opening a new one to cover contributors changes. We did the extra hook parsing in https://bugs.webkit.org/show_bug.cgi?id=235033.
Comment 23 Jonathan Bedard 2022-01-18 09:37:08 PST
Comment on attachment 448990 [details]
Patch

Won't be landing this, it was landed in 246007@main (r287978)
Comment 24 Jonathan Bedard 2022-01-18 13:30:47 PST
Testing automated comment (23)
Comment 25 Jonathan Bedard 2022-01-18 13:31:34 PST
Testing automated comment (24)
Comment 26 Jonathan Bedard 2022-01-19 10:08:21 PST
Testing comment response