Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement safe-merge-queue #17552

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Conversation

briannafan
Copy link
Contributor

@briannafan briannafan commented Sep 7, 2023

d3044ed

Implement safe-merge-queue
https://bugs.webkit.org/show_bug.cgi?id=261288
rdar://114993838

Reviewed by Jonathan Bedard.

Adding the safe-merge-queue label adds the PR to a queue which is periodically checked through BuildBot.
If the PR passes EWS checks, merge-queue label is automatically added and the PR will be landed.
If the label is not added by a committer, or if the PR does not pass EWS checks, merging-blocked label is added.

* Tools/CISupport/ews-build/config.json:
* Tools/CISupport/ews-build/factories.py:
(UnsafeMergeQueueFactory.__init__):
(SafeMergeQueueFactory):
(SafeMergeQueueFactory.__init__):
* Tools/CISupport/ews-build/factories_unittest.py:
(TestExpectedBuildSteps):
* Tools/CISupport/ews-build/loadConfig.py:
* Tools/CISupport/ews-build/steps.py:
(GitHubMixin):
(GitHubMixin.get_number_of_prs_with_label):
(ValidateCommitterAndReviewer):
(ValidateCommitterAndReviewer.fail_build):
(ValidateCommitterAndReviewer.run):
(DetermineLabelOwner.run):
(DetermineLabelOwner.getResultSummary):
(RemoveAndAddLabels):
(RemoveAndAddLabels.__init__):
(RemoveAndAddLabels.run):
(RemoveAndAddLabels.getResultSummary):
(RemoveAndAddLabels.update_labels):
(RetrievePRDataFromLabel):
(RetrievePRDataFromLabel.__init__):
(RetrievePRDataFromLabel.run):
(RetrievePRDataFromLabel.getResultSummary):
(RetrievePRDataFromLabel.getAllPRData):
(CheckStatusOfPR):
(CheckStatusOfPR.__init__):
(CheckStatusOfPR.run):
(CheckStatusOfPR.getResultSummary):
(CheckStatusOfPR.checkPRStatus):
(CheckStatusOfPR.getQueueStatusFromList):
(AddMergeLabelsToPRs):
(AddMergeLabelsToPRs.run):
(AddMergeLabelsToPRs.getResultSummary):
* Tools/CISupport/ews-build/steps_unittest.py:

Canonical link: https://commits.webkit.org/268847@main

7dd0e8f

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ§ͺ services βœ… πŸ›  tv-sim
βœ… πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@briannafan briannafan self-assigned this Sep 7, 2023
@briannafan briannafan added the Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases label Sep 7, 2023
{
"name": "Safe-Merge-Queue", "shortname": "safe-merge", "icon": "testOnly",
"factory": "SafeMergeQueueFactory", "platform": "*",
"workernames": ["ews151"]
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this is just for testing, the final version should be run on the same machines tasked with merge-queue.

Copy link
Member

Choose a reason for hiding this comment

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

Since we're nearing landing this, it's time to switch this to the correct worker names (ie, our merge-queue bots)

@@ -145,6 +146,7 @@ def credentials(cls, user=None):
return cls._cache[prefix]

try:
print("i bet there's no passwords.json")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove debugging statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch ^^"

query = {'query': query_body}
num_prs = self.queryGraphQL(query)['data']['repository']['pullRequests']['totalCount']
if num_prs:
yield self._addToLog('stdio', 'There are {} PRs in safe-merge-queue.'.format(num_prs))
Copy link
Member

Choose a reason for hiding this comment

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

Every function with a yield should be decorated with @defer.inlineCallbacks and every function invoking a function with @defer.inlineCallbacks needs to preface that invocation of with yield

return {'step': f"Failed to request and filter pull requests"}
return buildstep.BuildStep.getResultSummary(self)

def queryGraphQL(self, payload):
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be moved to the GitHub mixin class.

else:
return FAILURE
self.setProperty('safe_pr_list', label_as_safe)
self._addToLog('stdio', 'PRs to merge: {}.'.format(label_as_safe))
Copy link
Member

Choose a reason for hiding this comment

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

_addToLog is asynchronous and should be prefaced with yield

def test_success_sort(self):
self.setupStep(FilterPullRequestsIntoQueues())
FilterPullRequestsIntoQueues.getNumPRs = lambda label: 4
query_result = {'data': {'repository': {'pullRequests': {'edges': [{'node': {'title': 'Fix `test-webkitpy webkitflaskpy`', 'number': 17412, 'commits': {'nodes': [{'commit': {'commitUrl': 'https://github.com/WebKit/WebKit/commit/582fb8b4f85cc9f385c0e0809170cadc48c7fed5', 'status': {'state': 'SUCCESS', 'contexts': [{'context': 'api-gtk', 'state': 'SUCCESS'}, {'context': 'api-ios', 'state': 'SUCCESS'}, {'context': 'api-mac', 'state': 'SUCCESS'}, {'context': 'bindings', 'state': 'SUCCESS'}, {'context': 'gtk', 'state': 'SUCCESS'}, {'context': 'gtk-wk2', 'state': 'SUCCESS'}, {'context': 'ios', 'state': 'SUCCESS'}, {'context': 'ios-sim', 'state': 'SUCCESS'}, {'context': 'ios-wk2', 'state': 'SUCCESS'}, {'context': 'ios-wk2-wpt', 'state': 'SUCCESS'}, {'context': 'jsc', 'state': 'SUCCESS'}, {'context': 'jsc-arm64', 'state': 'SUCCESS'}, {'context': 'jsc-armv7', 'state': 'SUCCESS'}, {'context': 'jsc-i386', 'state': 'SUCCESS'}, {'context': 'jsc-mips', 'state': 'SUCCESS'}, {'context': 'mac', 'state': 'SUCCESS'}, {'context': 'mac-AS-debug', 'state': 'SUCCESS'}, {'context': 'mac-AS-debug-wk2', 'state': 'SUCCESS'}, {'context': 'mac-wk1', 'state': 'SUCCESS'}, {'context': 'mac-wk2', 'state': 'SUCCESS'}, {'context': 'mac-wk2-stress', 'state': 'SUCCESS'}, {'context': 'services', 'state': 'SUCCESS'}, {'context': 'style', 'state': 'SUCCESS'}, {'context': 'tv', 'state': 'SUCCESS'}, {'context': 'tv-sim', 'state': 'SUCCESS'}, {'context': 'watch', 'state': 'SUCCESS'}, {'context': 'watch-sim', 'state': 'SUCCESS'}, {'context': 'webkitperl', 'state': 'SUCCESS'}, {'context': 'webkitpy', 'state': 'SUCCESS'}, {'context': 'wincairo', 'state': 'SUCCESS'}, {'context': 'wpe', 'state': 'SUCCESS'}, {'context': 'wpe-wk2', 'state': 'SUCCESS'}]}}}]}}}, {'node': {'title': 'Import WPT css/compositing directory', 'number': 17418, 'commits': {'nodes': [{'commit': {'commitUrl': 'https://github.com/WebKit/WebKit/commit/b8df1771f6cb7197bcc8c3940670a24b8cd77d47', 'status': {'state': 'SUCCESS', 'contexts': [{'context': 'bindings', 'state': 'SUCCESS'}, {'context': 'ios', 'state': 'SUCCESS'}, {'context': 'jsc-arm64', 'state': 'SUCCESS'}, {'context': 'jsc-armv7', 'state': 'SUCCESS'}, {'context': 'jsc-i386', 'state': 'SUCCESS'}, {'context': 'jsc-mips', 'state': 'SUCCESS'}, {'context': 'services', 'state': 'SUCCESS'}, {'context': 'style', 'state': 'SUCCESS'}, {'context': 'tv', 'state': 'SUCCESS'}, {'context': 'watch-sim', 'state': 'SUCCESS'}, {'context': 'webkitperl', 'state': 'SUCCESS'}, {'context': 'webkitpy', 'state': 'SUCCESS'}, {'context': 'wpe', 'state': 'SUCCESS'}]}}}]}}}, {'node': {'title': 'Test safe-merge-queue labelling', 'number': 17451, 'commits': {'nodes': [{'commit': {'commitUrl': 'https://github.com/WebKit/WebKit/commit/b12b5ee6709e3ce6a9019342a12eded04975af18', 'status': {'state': 'FAILURE', 'contexts': [{'context': 'style', 'state': 'FAILURE'}, {'context': 'api-gtk', 'state': 'SUCCESS'}, {'context': 'api-ios', 'state': 'SUCCESS'}, {'context': 'api-mac', 'state': 'SUCCESS'}, {'context': 'bindings', 'state': 'SUCCESS'}, {'context': 'gtk', 'state': 'SUCCESS'}, {'context': 'ios', 'state': 'SUCCESS'}, {'context': 'ios-sim', 'state': 'SUCCESS'}, {'context': 'ios-wk2', 'state': 'SUCCESS'}, {'context': 'jsc', 'state': 'SUCCESS'}, {'context': 'jsc-arm64', 'state': 'SUCCESS'}, {'context': 'jsc-armv7', 'state': 'SUCCESS'}, {'context': 'jsc-i386', 'state': 'SUCCESS'}, {'context': 'jsc-mips', 'state': 'SUCCESS'}, {'context': 'mac', 'state': 'SUCCESS'}, {'context': 'mac-AS-debug', 'state': 'SUCCESS'}, {'context': 'mac-wk1', 'state': 'SUCCESS'}, {'context': 'mac-wk2', 'state': 'SUCCESS'}, {'context': 'mac-wk2-stress', 'state': 'SUCCESS'}, {'context': 'services', 'state': 'SUCCESS'}, {'context': 'tv', 'state': 'SUCCESS'}, {'context': 'tv-sim', 'state': 'SUCCESS'}, {'context': 'watch', 'state': 'SUCCESS'}, {'context': 'watch-sim', 'state': 'SUCCESS'}, {'context': 'webkitperl', 'state': 'SUCCESS'}, {'context': 'webkitpy', 'state': 'SUCCESS'}, {'context': 'wpe', 'state': 'SUCCESS'}]}}}]}}}, {'node': {'title': 'Split webkitpy.common.net.bugzilla.TestExpectationUpdater', 'number': 17454, 'commits': {'nodes': [{'commit': {'commitUrl': 'https://github.com/WebKit/WebKit/commit/4b36be2acc1b4b3190f93c69acd6484e58b35ef0', 'status': {'state': 'SUCCESS', 'contexts': [{'context': 'api-gtk', 'state': 'SUCCESS'}, {'context': 'bindings', 'state': 'SUCCESS'}, {'context': 'gtk', 'state': 'SUCCESS'}, {'context': 'ios', 'state': 'SUCCESS'}, {'context': 'ios-sim', 'state': 'SUCCESS'}, {'context': 'jsc-arm64', 'state': 'SUCCESS'}, {'context': 'jsc-armv7', 'state': 'SUCCESS'}, {'context': 'jsc-i386', 'state': 'SUCCESS'}, {'context': 'jsc-mips', 'state': 'SUCCESS'}, {'context': 'mac-AS-debug', 'state': 'SUCCESS'}, {'context': 'services', 'state': 'SUCCESS'}, {'context': 'style', 'state': 'SUCCESS'}, {'context': 'tv', 'state': 'SUCCESS'}, {'context': 'watch-sim', 'state': 'SUCCESS'}, {'context': 'webkitperl', 'state': 'SUCCESS'}, {'context': 'webkitpy', 'state': 'SUCCESS'}, {'context': 'wpe', 'state': 'SUCCESS'}]}}}]}}}]}}}}
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 organize this JSON a bit better? Maybe pretty-print it?

failed_status_check = []
no_checks = []

ignore_contexts = ['style', 'webkitperl', 'wpe', 'wpe-wk2', 'gtk', 'gtk-wk2', 'api-gtk', 'wincaire']
Copy link
Member

Choose a reason for hiding this comment

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

I think we want the reverse here: we want to list the tests we require to have passed before landing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that would just change the proceeding logic a little. But, wouldn't it be preferable if a new test was added and required by default? In the required list implementation, I could see a test added to EWS but not this list and then a PR with an error going into merge-queue.

Copy link
Member

Choose a reason for hiding this comment

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

New EWS queues usually start as experimental. The bigger sticking point, though, is that's it's possible for a build to be pending. In that case, we don't want safe-merge-queue to be triggered (yet). The pending case is challenging because there may not be a context reported yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll make the change. I'm currently putting all PRs with no status into its own list that's ignored, perhaps that takes care of pending builds?

@@ -193,6 +195,34 @@ def fetch_data_from_url_with_authentication_github(self, url):
else:
defer.returnValue(response)

@defer.inlineCallbacks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JonWBedard Tried implementing twistd requests here and moved the function to GitHubMixin as requested!

@briannafan briannafan added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Sep 8, 2023
@webkit-ews-buildbot webkit-ews-buildbot added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Sep 8, 2023
@webkit-ews-buildbot webkit-ews-buildbot added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Sep 8, 2023
@webkit-ews-buildbot webkit-ews-buildbot added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Sep 8, 2023
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Sep 28, 2023
@briannafan briannafan removed the merging-blocked Applied to prevent a change from being merged label Sep 28, 2023
@rniwa
Copy link
Member

rniwa commented Sep 30, 2023

What is safe-merge-queue and how it's different from regular merge-queue? That should be explained in the commit message.

def __init__(self, platform, configuration=None, architectures=None, additionalArguments=None, **kwargs):
super().__init__()
for project in GITHUB_PROJECTS:
self.addStep(RetrievePRDataFromLabel(project, label='\"safe-merge-queue\"'))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I don't think we should be quoting the label name here. And the label name should probably be a variable (as our other label names names are). I do really like the fact that this label is passed as an argument to RetrievePRDataFromLabel, so let's be suer to keep that!

@@ -236,6 +236,20 @@ def query_graph_ql(self, payload):
data = json.loads(response.text)
defer.returnValue(data)

@defer.inlineCallbacks
def get_number_of_prs_with_label(self, label='\"safe-merge-queue\"'):
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't have a default argument, since we only every call it by passing an argument (and I can't really see how future code would benefit from a default argument)

def __init__(self, project="WebKit/WebKit", label='\"safe-merge-queue\"', **kwargs):
self.project = project
owner, name = project.lower().split('/', 1)
self.name = f'retrieve-pr-data-from-{owner}-{name}'
Copy link
Member

Choose a reason for hiding this comment

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

This should be:

self.name = f'{self.name}-{owner}-{name}'

otherwise the class defined variable is completely useless.

QUEUES_FOR_SAFE_MERGE_QUEUE = EMBEDDED_CHECKS + MACOS_CHECKS

def __init__(self, pr_number='', **kwargs):
self.name = f'check-status-of-pr-{pr_number}-in-safe-merge-queue'
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on basing this on the class variable. Note that this does work in Python, and this is probably the right time to deploy this sort of tactic:

template_str = 'abc-{}-123'
resolved_str = template_str.format('def')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{} aren't allowed to be in Buildbot identifiers and makes unit tests fail. However, I think the step needs to be renamed anyways since it doesn't have to be safe-merge-queue specific, and it's only checking one PR, not multiple PRs. So now it's CheckStatusOfPR() with self.name = '{self.name}-{pr_number}'

Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

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

Implementation of safe-merge-queue label through Buildbot that filters PRs through EWS checks into merge-queue and merging-blocked.

This description is still not self explanatory. Please use a full sentence to explain it.

@briannafan
Copy link
Contributor Author

@rniwa There's still a bit of testing to do; I will update the commit message after that.

@JonWBedard JonWBedard requested a review from rniwa October 3, 2023 15:01
Copy link
Member

@JonWBedard JonWBedard left a comment

Choose a reason for hiding this comment

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

I want to collaborate with @briannafan on when to land and deploy, I am excited about this improvement!

@JonWBedard JonWBedard added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 4, 2023
https://bugs.webkit.org/show_bug.cgi?id=261288
rdar://114993838

Reviewed by Jonathan Bedard.

Adding the safe-merge-queue label adds the PR to a queue which is periodically checked through BuildBot.
If the PR passes EWS checks, merge-queue label is automatically added and the PR will be landed.
If the label is not added by a committer, or if the PR does not pass EWS checks, merging-blocked label is added.

* Tools/CISupport/ews-build/config.json:
* Tools/CISupport/ews-build/factories.py:
(UnsafeMergeQueueFactory.__init__):
(SafeMergeQueueFactory):
(SafeMergeQueueFactory.__init__):
* Tools/CISupport/ews-build/factories_unittest.py:
(TestExpectedBuildSteps):
* Tools/CISupport/ews-build/loadConfig.py:
* Tools/CISupport/ews-build/steps.py:
(GitHubMixin):
(GitHubMixin.get_number_of_prs_with_label):
(ValidateCommitterAndReviewer):
(ValidateCommitterAndReviewer.fail_build):
(ValidateCommitterAndReviewer.run):
(DetermineLabelOwner.run):
(DetermineLabelOwner.getResultSummary):
(RemoveAndAddLabels):
(RemoveAndAddLabels.__init__):
(RemoveAndAddLabels.run):
(RemoveAndAddLabels.getResultSummary):
(RemoveAndAddLabels.update_labels):
(RetrievePRDataFromLabel):
(RetrievePRDataFromLabel.__init__):
(RetrievePRDataFromLabel.run):
(RetrievePRDataFromLabel.getResultSummary):
(RetrievePRDataFromLabel.getAllPRData):
(CheckStatusOfPR):
(CheckStatusOfPR.__init__):
(CheckStatusOfPR.run):
(CheckStatusOfPR.getResultSummary):
(CheckStatusOfPR.checkPRStatus):
(CheckStatusOfPR.getQueueStatusFromList):
(AddMergeLabelsToPRs):
(AddMergeLabelsToPRs.run):
(AddMergeLabelsToPRs.getResultSummary):
* Tools/CISupport/ews-build/steps_unittest.py:

Canonical link: https://commits.webkit.org/268847@main
@webkit-commit-queue
Copy link
Collaborator

Committed 268847@main (d3044ed): https://commits.webkit.org/268847@main

Reviewed commits have been landed. Closing PR #17552 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 4, 2023
@webkit-commit-queue webkit-commit-queue merged commit d3044ed into WebKit:main Oct 4, 2023
@briannafan briannafan deleted the eng/114993838 branch December 12, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
7 participants