Skip to content

Conversation

@JonWBedard
Copy link
Member

@JonWBedard JonWBedard commented Jan 10, 2022

b04f96f

[EWS] Accept GitHub hooks (Part 1)
https://bugs.webkit.org/show_bug.cgi?id=235033


Reviewed by Aakash Jain

Trigger a subset of queues for incoming GitHub pull-requests.

* Tools/CISupport/ews-build/config.json: Enable webkitperl, webkitpy, services and style checks.
* Tools/CISupport/ews-build/loadConfig_unittest.py:
(ConfigDotJSONTest):
(ConfigDotJSONTest.test_multiple_scheduers_for_builder):
* Tools/CISupport/ews-build/master.cfg: Accept GitHub hooks, specify variables
to extract from incoming request.
* Tools/CISupport/ews-build/steps.py:
(GitHub):
(GitHub.repository_urls): Concatenate all valid projects with the base URL.
(GitHub.pr_url): Given a repository and pull-request number, return the URL
for that pull-request.
(ConfigureBuild.add_pr_details):
(GitHubMixin): Deleted.


Canonical link: https://commits.webkit.org/245978@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@287949 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@JonWBedard JonWBedard self-assigned this Jan 10, 2022
@JonWBedard JonWBedard marked this pull request as draft January 10, 2022 17:39
@JonWBedard
Copy link
Member Author

Not ready for landing yet, wanted to post this code for reference.

@JonWBedard JonWBedard force-pushed the eng/github-ews-enable branch from 72e1369 to 00992e5 Compare January 12, 2022 17:19
@JonWBedard JonWBedard marked this pull request as ready for review January 12, 2022 17:20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this scheduler triggered?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub hooks generate by pull-requests have this:
-H "X-GitHub-Event: pull_request
in their header. My understanding is that upon receiving this hook, buildbot triggers the scheduler named pull request

@JonWBedard JonWBedard force-pushed the eng/github-ews-enable branch 2 times, most recently from 424a176 to 3ee2807 Compare January 12, 2022 17:28
Copy link
Member

@aj062 aj062 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. few minor comments.
Also one unit-test is failing, need to delete/update it https://ews-build.webkit.org/#/builders/20/builds/65384

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still unclear who is setting this 'repository' property. Can we just skip it altogether?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'repository' comes from the GitHub hook, Making it optional may be reasonable, but we don't want to drop it entirely. This is one of the arguments that will change for security pull-requests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do this:
def pr_url(cls, pr_number, repository_url=None):

@JonWBedard JonWBedard force-pushed the eng/github-ews-enable branch 2 times, most recently from 084f266 to 9a1636d Compare January 12, 2022 20:05
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if condition seems incorrect, please use explicit parenthesis to make it correct and clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I preferred previous one-liner, but this is ok as well.

@aj062
Copy link
Member

aj062 commented Jan 12, 2022

r=me (with above comments)

@JonWBedard JonWBedard force-pushed the eng/github-ews-enable branch 2 times, most recently from 76b74ec to 8167e2b Compare January 12, 2022 21:16
@JonWBedard JonWBedard changed the title [EWS] Accept GitHub hooks (Part 1) [ iOS EWS ] imported/w3c/web-platform-tests/dom/events/focus-event-document-move.html is a constant text failure Jan 12, 2022
@JonWBedard JonWBedard closed this Jan 12, 2022
@JonWBedard JonWBedard force-pushed the eng/github-ews-enable branch from 8167e2b to c857ce6 Compare January 12, 2022 21:18
@JonWBedard JonWBedard merged commit c857ce6 into WebKit:main Jan 12, 2022
@JonWBedard
Copy link
Member Author

JonWBedard commented Jan 12, 2022

@JonWBedard JonWBedard deleted the eng/github-ews-enable branch January 12, 2022 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants