Bug 234848 – [EWS] Support pull-requests in ConfigureBuild

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

[Jonathan Bedard]

The ConfigureBuild step should support GitHub pull-requests.

[Radar WebKit Bug Importer]

<rdar://problem/87094989>

[Jonathan Bedard]

PR: https://github.com/WebKit/WebKit/pull/61

[Jonathan Bedard]

Created attachment 448307 [details]
Patch

[Jonathan Bedard]

Created attachment 448309 [details]
Patch

[Aakash Jain]

Comment on attachment 448309 [details]
Patch

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

> Tools/CISupport/ews-build/steps.py:79
> +        return '{}/pull/{}'.format(GITHUB_URL, pr_number)

pr_number isn't defined in this method

> Tools/CISupport/ews-build/steps.py:83
> +    def pull_request(self):

method name seems too generic and doesn't indicate what it might be doing, maybe something like get_pull_request_number might be more informative.

> Tools/CISupport/ews-build/steps.py:86
> +            return int(pull_request)

better to call this pr_number to indicate that it is an integer rather than an object. Also, probably need to gracefully handle the case when conversion to int raises an exception

> Tools/CISupport/ews-build/steps.py:88
> +        if self.getProperty('event') != 'pull_request':

who is setting these 'event' and 'branch' properties?

> Tools/CISupport/ews-build/steps.py:93
> +        self.setProperty('pull_request', match.group('id'))

Is the regex guaranteed to contain the PR number and that would be guaranteed to be integer?
It might be good idea to validate that this can be cast to an int before setting the property (since we will try to read it as a int later on)

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

I think it would be better to display "Pull Request" instead of "Pull-reqeust"

[Jonathan Bedard]

Comment on attachment 448309 [details]
Patch

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

>> Tools/CISupport/ews-build/steps.py:88
>> +        if self.getProperty('event') != 'pull_request':
> 
> who is setting these 'event' and 'branch' properties?

That comes from buildbot's digestion of GitHub hooks. http://docs.buildbot.net/current/manual/configuration/wwwhooks.html outlines some of the basics

>> Tools/CISupport/ews-build/steps.py:93
>> +        self.setProperty('pull_request', match.group('id'))
> 
> Is the regex guaranteed to contain the PR number and that would be guaranteed to be integer?
> It might be good idea to validate that this can be cast to an int before setting the property (since we will try to read it as a int later on)

The regex forces id to be an integer, so we wouldn't have a match if it wasn't an integer.

I was under the impression properties needed to be strings, but looking at some other builds, that doesn't look to be true.

[Jonathan Bedard]

Created attachment 448332 [details]
Patch

[Jonathan Bedard]

Created attachment 448340 [details]
Patch

[Jonathan Bedard]

Landed 245802@main (r287719)