Bug 235074 – [EWS] Load contributors from stand-alone class

• Component: Tools / Tests
• Status: RESOLVED CONFIGURATION CHANGED

[Jonathan Bedard]

The hook that GitHub sends buildbot has all the information we need, we should parse it's output into build properties.

[Radar WebKit Bug Importer]

<rdar://problem/87406157>

[Jonathan Bedard]

Pull-request: https://github.com/WebKit/WebKit/pull/69

[Jonathan Bedard]

Created attachment 448854 [details]
Patch

[Jonathan Bedard]

Created attachment 448882 [details]
Patch

[Aakash Jain]

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)

[Aakash Jain]

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.

[Jonathan Bedard]

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?

[Jonathan Bedard]

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>'

[Jonathan Bedard]

Created attachment 448947 [details]
Patch

[Jonathan Bedard]

Created attachment 448948 [details]
Patch

[Aakash Jain]

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

[Jonathan Bedard]

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

[Jonathan Bedard]

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

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 448963 [details]
Patch

[Jonathan Bedard]

Created attachment 448968 [details]
Patch

[Jonathan Bedard]

Created attachment 448974 [details]
Patch

[Jonathan Bedard]

Created attachment 448975 [details]
Patch

[Jonathan Bedard]

Created attachment 448981 [details]
Patch

[Jonathan Bedard]

Created attachment 448983 [details]
Patch

[Jonathan Bedard]

Created attachment 448990 [details]
Patch

[Jonathan Bedard]

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.

[Jonathan Bedard]

Comment on attachment 448990 [details]
Patch

Won't be landing this, it was landed in 246007@main (r287978)

[Jonathan Bedard]

Testing automated comment (23)

[Jonathan Bedard]

Testing automated comment (24)

[Jonathan Bedard]

Testing comment response