Diff
Modified: trunk/Tools/CISupport/ews-build/factories.py (291618 => 291619)
--- trunk/Tools/CISupport/ews-build/factories.py 2022-03-22 15:24:09 UTC (rev 291618)
+++ trunk/Tools/CISupport/ews-build/factories.py 2022-03-22 15:25:30 UTC (rev 291619)
@@ -32,7 +32,7 @@
RunEWSUnitTests, RunResultsdbpyTests, RunJavaScriptCoreTests, RunWebKit1Tests, RunWebKitPerlTests, RunWebKitPyPython2Tests,
RunWebKitPyPython3Tests, RunWebKitTests, RunWebKitTestsRedTree, RunWebKitTestsInStressMode, RunWebKitTestsInStressGuardmallocMode,
SetBuildSummary, ShowIdentifier, TriggerCrashLogSubmission, UpdateWorkingDirectory,
- ValidateChange, ValidateChangeLogAndReviewer, ValidateCommiterAndReviewer, WaitForCrashCollection,
+ ValidateChange, ValidateChangeLogAndReviewer, ValidateCommitterAndReviewer, WaitForCrashCollection,
InstallBuiltProduct, VerifyGitHubIntegrity)
@@ -289,7 +289,7 @@
factory.BuildFactory.__init__(self)
self.addStep(ConfigureBuild(platform=platform, configuration=configuration, architectures=architectures, buildOnly=False, triggers=None, remotes=None, additionalArguments=additionalArguments))
self.addStep(ValidateChange(verifycqplus=True))
- self.addStep(ValidateCommiterAndReviewer())
+ self.addStep(ValidateCommitterAndReviewer())
self.addStep(PrintConfiguration())
self.addStep(CleanGitRepo(default_branch='master'))
self.addStep(CheckOutSource(repourl='https://git.webkit.org/git/WebKit-https'))
@@ -321,3 +321,12 @@
factory.BuildFactory.__init__(self)
self.addStep(ConfigureBuild(platform=platform, configuration=configuration, architectures=architectures, buildOnly=False, triggers=None, remotes=None, additionalArguments=additionalArguments))
self.addStep(ValidateChange(verifyMergeQueue=True, verifyNoDraftForMergeQueue=True))
+ self.addStep(ValidateCommitterAndReviewer())
+ self.addStep(PrintConfiguration())
+ self.addStep(CleanGitRepo())
+ self.addStep(CheckOutSource())
+ self.addStep(FetchBranches())
+ self.addStep(ShowIdentifier())
+ self.addStep(VerifyGitHubIntegrity())
+ self.addStep(UpdateWorkingDirectory())
+ self.addStep(CheckOutPullRequest())
Modified: trunk/Tools/CISupport/ews-build/factories_unittest.py (291618 => 291619)
--- trunk/Tools/CISupport/ews-build/factories_unittest.py 2022-03-22 15:24:09 UTC (rev 291618)
+++ trunk/Tools/CISupport/ews-build/factories_unittest.py 2022-03-22 15:25:30 UTC (rev 291619)
@@ -629,6 +629,15 @@
'Merge-Queue': [
'configure-build',
'validate-change',
+ 'validate-commiter-and-reviewer',
+ 'configuration',
+ 'clean-up-git-repo',
+ 'clean-and-update-working-directory',
+ 'fetch-branch-references',
+ 'show-identifier',
+ 'verify-github-integrity',
+ 'update-working-directory',
+ 'checkout-pull-request'
],
}
Modified: trunk/Tools/CISupport/ews-build/steps.py (291618 => 291619)
--- trunk/Tools/CISupport/ews-build/steps.py 2022-03-22 15:24:09 UTC (rev 291618)
+++ trunk/Tools/CISupport/ews-build/steps.py 2022-03-22 15:25:30 UTC (rev 291619)
@@ -183,12 +183,29 @@
self._addToLog('stdio', 'Unable to fetch pull request {}.\n'.format(pr_number))
if attempt > retry:
return None
- wait_for = (index + 1) * 15
+ wait_for = (attempt + 1) * 15
self._addToLog('stdio', 'Backing off for {} seconds before retrying.\n'.format(wait_for))
time.sleep(wait_for)
return None
+ def get_reviewers(self, pr_number, repository_url=None):
+ api_url = GitHub.api_url(repository_url)
+ if not api_url:
+ return []
+
+ reviews_url = f'{api_url}/pulls/{pr_number}/reviews'
+ content = self.fetch_data_from_url_with_authentication(reviews_url)
+ if not content:
+ return []
+
+ result = []
+ for review in (content.json() or []):
+ reviewer = review.get('user', {}).get('login')
+ if reviewer and review.get('state') == 'APPROVED':
+ result.append(reviewer)
+ return result
+
def _is_pr_closed(self, pr_json):
if not pr_json or not pr_json.get('state'):
self._addToLog('stdio', 'Cannot determine pull request status.\n')
@@ -1445,12 +1462,12 @@
return True
-class ValidateCommiterAndReviewer(buildstep.BuildStep):
+class ValidateCommitterAndReviewer(buildstep.BuildStep, GitHubMixin):
name = 'validate-commiter-and-reviewer'
descriptionDone = ['Validated commiter and reviewer']
def __init__(self, *args, **kwargs):
- super(ValidateCommiterAndReviewer, self).__init__(*args, **kwargs)
+ super(ValidateCommitterAndReviewer, self).__init__(*args, **kwargs)
self.contributors = {}
@defer.inlineCallbacks
@@ -1466,15 +1483,21 @@
return {'step': self.descriptionDone}
return buildstep.BuildStep.getResultSummary(self)
- def fail_build(self, email, status):
- reason = '{} does not have {} permissions'.format(email, status)
- comment = '{} does not have {} permissions according to {}.'.format(email, status, Contributors.url)
- comment += '\n\nRejecting attachment {} from commit queue.'.format(self.getProperty('patch_id', ''))
+ def fail_build(self, email_or_username, status):
+ patch_id = self.getProperty('patch_id', '')
+ pr_number = self.getProperty('github.number', '')
+
+ reason = f'{email_or_username} does not have {status} permissions'
+ comment = f'{"@" if pr_number else ""}{email_or_username} does not have {status} permissions according to {Contributors.url}.'
+ if patch_id:
+ comment += f'\n\nRejecting attachment {patch_id} from commit queue.'
+ elif pr_number:
+ comment += f'\n\nRejecting {self.getProperty("github.head.sha", f"#{pr_number}")} from merge queue.'
self.setProperty('comment_text', comment)
self._addToLog('stdio', reason)
self.setProperty('build_finish_summary', reason)
- self.build.addStepsAfterCurrentStep([LeaveComment(), SetCommitQueueMinusFlagOnPatch()])
+ self.build.addStepsAfterCurrentStep([LeaveComment(), BlockPullRequest(), SetCommitQueueMinusFlagOnPatch()])
self.finished(FAILURE)
self.descriptionDone = reason
@@ -1503,24 +1526,40 @@
self.descriptionDone = 'Failed to get contributors information'
self.build.buildFinished(['Failed to get contributors information'], FAILURE)
return None
- patch_committer = self.getProperty('patch_committer', '').lower()
- if not self.is_committer(patch_committer):
- self.fail_build(patch_committer, 'committer')
+
+ pr_number = self.getProperty('github.number', '')
+
+ if pr_number:
+ committer = (self.getProperty('owners', []) or [''])[0]
+ else:
+ committer = self.getProperty('patch_committer', '').lower()
+
+ if not self.is_committer(committer):
+ self.fail_build(committer, 'committer')
return None
- self._addToLog('stdio', '{} is a valid commiter.\n'.format(patch_committer))
+ self._addToLog('stdio', f'{committer} is a valid commiter.\n')
- reviewer = self.getProperty('reviewer', '').lower()
- if not reviewer:
- # Patch does not have r+ flag. This is acceptable, since the ChangeLog might have 'Reviewed by' in it.
+ if pr_number:
+ reviewers = self.get_reviewers(pr_number, self.getProperty('repository', ''))
+ if any([self.is_reviewer(reviewer) for reviewer in reviewers]):
+ reviewers = list(filter(self.is_reviewer, reviewers))
+ else:
+ reviewer = self.getProperty('reviewer', '').lower()
+ reviewers = [reviewer] if reviewer else []
+
+ if not reviewers:
+ # Change has not been reviewed in bug tracker. This is acceptable, since the ChangeLog might have 'Reviewed by' in it.
self.descriptionDone = 'Validated committer'
self.finished(SUCCESS)
return None
- self.setProperty('reviewers_full_names', [self.full_name_from_email(reviewer)])
- if not self.is_reviewer(reviewer):
- self.fail_build(reviewer, 'reviewer')
- return None
- self._addToLog('stdio', '{} is a valid reviewer.\n'.format(reviewer))
+ for reviewer in reviewers:
+ if not self.is_reviewer(reviewer):
+ self.fail_build(reviewer, 'reviewer')
+ return None
+ self._addToLog('stdio', f'{reviewer} is a valid reviewer.\n')
+ self.setProperty('reviewers_full_names', [self.full_name_from_email(reviewer) for reviewer in reviewers])
+
self.finished(SUCCESS)
return None
Modified: trunk/Tools/CISupport/ews-build/steps_unittest.py (291618 => 291619)
--- trunk/Tools/CISupport/ews-build/steps_unittest.py 2022-03-22 15:24:09 UTC (rev 291618)
+++ trunk/Tools/CISupport/ews-build/steps_unittest.py 2022-03-22 15:25:30 UTC (rev 291619)
@@ -56,7 +56,7 @@
RunWebKitTestsWithoutChange, RunWebKitTestsRedTree, RunWebKitTestsRepeatFailuresRedTree, RunWebKitTestsRepeatFailuresWithoutChangeRedTree,
RunWebKitTestsWithoutChangeRedTree, AnalyzeLayoutTestsResultsRedTree, TestWithFailureCount, ShowIdentifier,
Trigger, TransferToS3, UnApplyPatch, UpdateWorkingDirectory, UploadBuiltProduct,
- UploadTestResults, ValidateChangeLogAndReviewer, ValidateCommiterAndReviewer, ValidateChange, VerifyGitHubIntegrity)
+ UploadTestResults, ValidateChangeLogAndReviewer, ValidateCommitterAndReviewer, ValidateChange, VerifyGitHubIntegrity)
# Workaround for https://github.com/buildbot/buildbot/issues/4669
from buildbot.test.fake.fakebuild import FakeBuild
@@ -5157,13 +5157,18 @@
return rc
-class TestValidateCommiterAndReviewer(BuildStepMixinAdditions, unittest.TestCase):
+class TestValidateCommitterAndReviewer(BuildStepMixinAdditions, unittest.TestCase):
def setUp(self):
self.longMessage = True
def mock_load_contributors(*args, **kwargs):
- return {'aakash_j...@apple.com': {'name': 'Aakash Jain', 'status': 'reviewer'},
- 'commit...@webkit.org': {'name': 'WebKit Committer', 'status': 'committer'}}, []
+ return {
+ 'aakash_j...@apple.com': {'name': 'Aakash Jain', 'status': 'reviewer'},
+ 'jain-aakash': {'name': 'Aakash Jain', 'status': 'reviewer'},
+ 'commit...@webkit.org': {'name': 'WebKit Committer', 'status': 'committer'},
+ 'webkit-commit-queue': {'name': 'WebKit Committer', 'status': 'committer'},
+ }, []
+
Contributors.load = mock_load_contributors
return self.setUpBuildStep()
@@ -5170,18 +5175,28 @@
def tearDown(self):
return self.tearDownBuildStep()
- def test_success(self):
- self.setupStep(ValidateCommiterAndReviewer())
+ def test_success_patch(self):
+ self.setupStep(ValidateCommitterAndReviewer())
self.setProperty('patch_id', '1234')
self.setProperty('patch_committer', 'commit...@webkit.org')
self.setProperty('reviewer', 'aakash_j...@apple.com')
self.expectHidden(False)
- self.assertEqual(ValidateCommiterAndReviewer.haltOnFailure, False)
+ self.assertEqual(ValidateCommitterAndReviewer.haltOnFailure, False)
self.expectOutcome(result=SUCCESS, state_string='Validated commiter and reviewer')
return self.runStep()
- def test_success_no_reviewer(self):
- self.setupStep(ValidateCommiterAndReviewer())
+ def test_success_pr(self):
+ self.setupStep(ValidateCommitterAndReviewer())
+ ValidateCommitterAndReviewer.get_reviewers = lambda x, pull_request, repository_url=None: ['jain-aakash']
+ self.setProperty('github.number', '1234')
+ self.setProperty('owners', ['webkit-commit-queue'])
+ self.expectHidden(False)
+ self.assertEqual(ValidateCommitterAndReviewer.haltOnFailure, False)
+ self.expectOutcome(result=SUCCESS, state_string='Validated commiter and reviewer')
+ return self.runStep()
+
+ def test_success_no_reviewer_patch(self):
+ self.setupStep(ValidateCommitterAndReviewer())
self.setProperty('patch_id', '1234')
self.setProperty('patch_committer', 'aakash_j...@apple.com')
self.expectHidden(False)
@@ -5188,8 +5203,17 @@
self.expectOutcome(result=SUCCESS, state_string='Validated committer')
return self.runStep()
- def test_failure_load_contributors(self):
- self.setupStep(ValidateCommiterAndReviewer())
+ def test_success_no_reviewer_pr(self):
+ self.setupStep(ValidateCommitterAndReviewer())
+ ValidateCommitterAndReviewer.get_reviewers = lambda x, pull_request, repository_url=None: []
+ self.setProperty('github.number', '1234')
+ self.setProperty('owners', ['jain-aakash'])
+ self.expectHidden(False)
+ self.expectOutcome(result=SUCCESS, state_string='Validated committer')
+ return self.runStep()
+
+ def test_failure_load_contributors_patch(self):
+ self.setupStep(ValidateCommitterAndReviewer())
self.setProperty('patch_id', '1234')
self.setProperty('patch_committer', 'a...@webkit.org')
Contributors.load = lambda: ({}, [])
@@ -5197,8 +5221,17 @@
self.expectOutcome(result=FAILURE, state_string='Failed to get contributors information')
return self.runStep()
- def test_failure_invalid_committer(self):
- self.setupStep(ValidateCommiterAndReviewer())
+ def test_failure_load_contributors_pr(self):
+ self.setupStep(ValidateCommitterAndReviewer())
+ self.setProperty('github.number', '1234')
+ self.setProperty('owners', ['abc'])
+ Contributors.load = lambda: ({}, [])
+ self.expectHidden(False)
+ self.expectOutcome(result=FAILURE, state_string='Failed to get contributors information')
+ return self.runStep()
+
+ def test_failure_invalid_committer_patch(self):
+ self.setupStep(ValidateCommitterAndReviewer())
self.setProperty('patch_id', '1234')
self.setProperty('patch_committer', 'a...@webkit.org')
self.expectHidden(False)
@@ -5205,8 +5238,16 @@
self.expectOutcome(result=FAILURE, state_string='a...@webkit.org does not have committer permissions')
return self.runStep()
- def test_failure_invalid_reviewer(self):
- self.setupStep(ValidateCommiterAndReviewer())
+ def test_failure_invalid_committer_pr(self):
+ self.setupStep(ValidateCommitterAndReviewer())
+ self.setProperty('github.number', '1234')
+ self.setProperty('owners', ['abc'])
+ self.expectHidden(False)
+ self.expectOutcome(result=FAILURE, state_string='abc does not have committer permissions')
+ return self.runStep()
+
+ def test_failure_invalid_reviewer_patch(self):
+ self.setupStep(ValidateCommitterAndReviewer())
self.setProperty('patch_id', '1234')
self.setProperty('patch_committer', 'aakash_j...@apple.com')
self.setProperty('reviewer', 'commit...@webkit.org')
@@ -5214,6 +5255,15 @@
self.expectOutcome(result=FAILURE, state_string='commit...@webkit.org does not have reviewer permissions')
return self.runStep()
+ def test_failure_invalid_reviewer_pr(self):
+ self.setupStep(ValidateCommitterAndReviewer())
+ ValidateCommitterAndReviewer.get_reviewers = lambda x, pull_request, repository_url=None: ['webkit-commit-queue']
+ self.setProperty('github.number', '1234')
+ self.setProperty('owners', ['jain-aakash'])
+ self.expectHidden(False)
+ self.expectOutcome(result=FAILURE, state_string='webkit-commit-queue does not have reviewer permissions')
+ return self.runStep()
+
def test_load_contributors_from_disk(self):
contributors = filter(lambda element: element.get('name') == 'Aakash Jain', Contributors().load_from_disk()[0])
self.assertEqual(list(contributors)[0]['emails'][0], 'aakash_j...@apple.com')
Modified: trunk/Tools/ChangeLog (291618 => 291619)
--- trunk/Tools/ChangeLog 2022-03-22 15:24:09 UTC (rev 291618)
+++ trunk/Tools/ChangeLog 2022-03-22 15:25:30 UTC (rev 291619)
@@ -1,3 +1,29 @@
+2022-03-21 Jonathan Bedard <jbed...@apple.com>
+
+ [Merge-Queue] Add ValidateCommitterAndReviewer
+ https://bugs.webkit.org/show_bug.cgi?id=238150
+ <rdar://problem/90587620>
+
+ Reviewed by Aakash Jain.
+
+ * CISupport/ews-build/factories.py:
+ (CommitQueueFactory.__init__): Rename ValidateCommiterAndReviewer to ValidateCommiterAndReviewer.
+ (MergeQueueFactory.__init__): Add ValidateCommitterAndReviewer, PrintConfiguration, CleanGitRepo,
+ CheckOutSource, FetchBranches, ShowIdentifier, VerifyGitHubIntegrity, UpdateWorkingDirectory
+ and CheckOutPullRequest.
+ * CISupport/ews-build/factories_unittest.py:
+ (TestExpectedBuildSteps): Add Merge-Queue step names.
+ * CISupport/ews-build/steps.py:
+ (GitHubMixin.get_pr_json): Retry tied to attempt not index.
+ (GitHubMixin.get_reviewers): Get list of reviewers for pull-request in repository.
+ (ValidateCommitterAndReviewer): Renamed from ValidateCommiterAndReviewer, inherit from GitHubMixin.
+ (ValidateCommitterAndReviewer.__init__): Rename ValidateCommiterAndReviewer.
+ (ValidateCommitterAndReviewer.fail_build): Block pull request, differentiate comment for pull
+ request and patch.
+ (ValidateCommitterAndReviewer.start): Support pull requests.
+ (ValidateCommiterAndReviewer): Renamed to ValidateCommitterAndReviewer.
+ * CISupport/ews-build/steps_unittest.py:
+
2022-03-22 Wenson Hsieh <wenson_hs...@apple.com>
-[WKWebView _spellCheckerDocumentTag] is inconsistent with the document tag passed into NSSpellChecker
