Bug 222050 - [git-webkit] Failure to retrieve commit in EWS
Summary: [git-webkit] Failure to retrieve commit in EWS
Status: RESOLVED FIXED
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: 2021-02-17 10:37 PST by Jonathan Bedard
Modified: 2021-02-23 05:09 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.32 KB, patch)
2021-02-17 10:41 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.68 KB, patch)
2021-02-17 16:45 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2021-02-18 10:07 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2021-02-18 11:15 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (3.94 KB, patch)
2021-02-22 10:30 PST, Jonathan Bedard
aakash_jain: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2021-02-17 10:37:46 PST
Occasionally, EWS is failing to retrieve commit information. We need to determine why. An example of this failure is: https://ews-build.webkit.org/#/builders/36/builds/28373.
Comment 1 Radar WebKit Bug Importer 2021-02-17 10:38:28 PST
<rdar://problem/74439957>
Comment 2 Jonathan Bedard 2021-02-17 10:41:01 PST
Created attachment 420668 [details]
Patch
Comment 3 Aakash Jain 2021-02-17 10:44:22 PST
Comment on attachment 420668 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:153
> +            sys.stderr.write("Request to '{}' returned status code '{}'".format(url, response.status_code))

Do we need to add '\n'?
Comment 4 Jonathan Bedard 2021-02-17 11:14:14 PST
Committed r273015 (234216@main): <https://commits.webkit.org/234216@main>
Comment 5 Jonathan Bedard 2021-02-17 16:45:33 PST
Reopening to attach new patch.
Comment 6 Jonathan Bedard 2021-02-17 16:45:35 PST
Created attachment 420759 [details]
Patch
Comment 7 Aakash Jain 2021-02-17 17:28:54 PST
Comment on attachment 420759 [details]
Patch

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

r+ with unit-tests fixed.

> Tools/CISupport/ews-build/loadConfig.py:54
> +        value = passwords.get(variable.lower().replace('_', '-'))

why replace?

> Tools/CISupport/ews-build/steps.py:55
> +GITHUB_COM_USERNAME = 'GITHUB_COM_USERNAME'

can rename this to GITHUB_USERNAME to be more readable.
Comment 8 Jonathan Bedard 2021-02-18 10:07:03 PST
Created attachment 420840 [details]
Patch
Comment 9 Aakash Jain 2021-02-18 10:12:40 PST
Comment on attachment 420840 [details]
Patch

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

> Tools/CISupport/ews-build/loadConfig.py:54
> +        value = passwords.get(variable.lower().replace('_', '-'))

why replace? why not just have GITHUB_USERNAME listed in passwords.json?
Comment 10 Jonathan Bedard 2021-02-18 10:34:14 PST
(In reply to Aakash Jain from comment #9)
> Comment on attachment 420840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420840&action=review
> 
> > Tools/CISupport/ews-build/loadConfig.py:54
> > +        value = passwords.get(variable.lower().replace('_', '-'))
> 
> why replace? why not just have GITHUB_USERNAME listed in passwords.json?

Was trying to match what we did for the results database's API key, I'm fine with the raw variable in passwords.json.
Comment 11 Jonathan Bedard 2021-02-18 11:15:08 PST
Created attachment 420854 [details]
Patch
Comment 12 Aakash Jain 2021-02-19 17:40:40 PST
Comment on attachment 420854 [details]
Patch

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

> Tools/CISupport/ews-build/loadConfig.py:53
> +    for variable in ['GITHUB_COM_USERNAME', 'GITHUB_COM_ACCESS_TOKEN']:

I prefer GITHUB_USERNAME everywhere for better readability, but that’s not a big deal.
Comment 13 Aakash Jain 2021-02-19 17:41:02 PST
Maybe we should also test this on uat instance.
Comment 14 Jonathan Bedard 2021-02-22 08:48:22 PST
(In reply to Aakash Jain from comment #12)
> Comment on attachment 420854 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420854&action=review
> 
> > Tools/CISupport/ews-build/loadConfig.py:53
> > +    for variable in ['GITHUB_COM_USERNAME', 'GITHUB_COM_ACCESS_TOKEN']:
> 
> I prefer GITHUB_USERNAME everywhere for better readability, but that’s not a
> big deal.

Changing the environment variable would result in a bit of a mess for things already using it (like commits.webkit.org). The environment variable is derived from the domain name of the GitHub instance.
Comment 15 Aakash Jain 2021-02-22 09:07:58 PST
(In reply to Jonathan Bedard from comment #14)
> Changing the environment variable would result in a bit of a mess for things already using it (like commits.webkit.org). The environment variable is derived from the domain name of the GitHub instance.
I didn't know that. Let's use GITHUB_COM_USERNAME and GITHUB_COM_ACCESS_TOKEN everywhere than, to maintain consistency.
Comment 16 Jonathan Bedard 2021-02-22 10:30:30 PST
Created attachment 421209 [details]
Patch
Comment 17 Jonathan Bedard 2021-02-22 12:01:06 PST
Committed r273267 (234438@main): <https://commits.webkit.org/234438@main>
Comment 18 Aakash Jain 2021-02-23 05:09:30 PST
(In reply to Jonathan Bedard from comment #17)
> Committed r273267 (234438@main): <https://commits.webkit.org/234438@main>
Restarted buildbot to pick up this change.