The hook that GitHub sends buildbot has all the information we need, we should parse it's output into build properties.
<rdar://problem/87406157>
Pull-request: https://github.com/WebKit/WebKit/pull/69
Created attachment 448854 [details] Patch
Created attachment 448882 [details] Patch
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)
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 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 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>'
Created attachment 448947 [details] Patch
Created attachment 448948 [details] Patch
(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"
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
(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.
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.
Created attachment 448963 [details] Patch
Created attachment 448968 [details] Patch
Created attachment 448974 [details] Patch
Created attachment 448975 [details] Patch
Created attachment 448981 [details] Patch
Created attachment 448983 [details] Patch
Created attachment 448990 [details] Patch
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 on attachment 448990 [details] Patch Won't be landing this, it was landed in 246007@main (r287978)
Testing automated comment (23)
Testing automated comment (24)
Testing comment response