The ConfigureBuild step should support GitHub pull-requests.
<rdar://problem/87094989>
PR: https://github.com/WebKit/WebKit/pull/61
Created attachment 448307 [details] Patch
Created attachment 448309 [details] Patch
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"
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.
Created attachment 448332 [details] Patch
Created attachment 448340 [details] Patch
Landed 245802@main (r287719)