From 27b677df914abfbc145c03549a9caff54b093316 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 23 Sep 2021 15:44:12 -0700 Subject: [PATCH] Only refresh deps if change messages have changed We only need to call the refreshDeps method if the Depends-On list has changed. That can only happen with a new patchset (gerrit) or the PR body has changed (github et al). Add a method to determine if the PR body has changed so we can reduce the times where we need to call this method. Change-Id: Iaa50a274c29347397bc4e10e2c3cefc25e442879 --- tests/base.py | 11 ++++++--- tests/unit/test_cross_crd.py | 31 ++++++++++++-------------- tests/unit/test_github_crd.py | 3 +-- tests/unit/test_pagure_driver.py | 5 +---- zuul/driver/github/githubconnection.py | 2 ++ zuul/driver/github/githubmodel.py | 6 +++++ zuul/driver/gitlab/gitlabconnection.py | 2 ++ zuul/driver/gitlab/gitlabmodel.py | 8 +++++++ zuul/driver/pagure/pagureconnection.py | 1 + zuul/driver/pagure/paguremodel.py | 6 +++++ zuul/model.py | 3 +++ zuul/scheduler.py | 7 ++---- 12 files changed, 54 insertions(+), 31 deletions(-) diff --git a/tests/base.py b/tests/base.py index f721e01555..eb76ab6664 100644 --- a/tests/base.py +++ b/tests/base.py @@ -2412,8 +2412,8 @@ class FakeGithubPullRequest(object): def getPullRequestClosedEvent(self): return self._getPullRequestEvent('closed') - def getPullRequestEditedEvent(self): - return self._getPullRequestEvent('edited') + def getPullRequestEditedEvent(self, old_body=None): + return self._getPullRequestEvent('edited', old_body) def addComment(self, message): self.comments.append(message) @@ -2546,8 +2546,10 @@ class FakeGithubPullRequest(object): return (name, data) def editBody(self, body): + old_body = self.body self.body = body self._updateTimeStamp() + return self.getPullRequestEditedEvent(old_body=old_body) def _getRepo(self): repo_path = os.path.join(self.upstream_root, self.project) @@ -2636,7 +2638,7 @@ class FakeGithubPullRequest(object): def getPRReference(self): return '%s/head' % self.number - def _getPullRequestEvent(self, action): + def _getPullRequestEvent(self, action, old_body=None): name = 'pull_request' data = { 'action': action, @@ -2668,8 +2670,11 @@ class FakeGithubPullRequest(object): 'installation': { 'id': 123, }, + 'changes': {}, 'labels': [{'name': l} for l in self.labels] } + if old_body: + data['changes']['body'] = {'from': old_body} return (name, data) def getCommitStatusEvent(self, context, state='success', user='zuul'): diff --git a/tests/unit/test_cross_crd.py b/tests/unit/test_cross_crd.py index e42a460ffb..9e04241f29 100644 --- a/tests/unit/test_cross_crd.py +++ b/tests/unit/test_cross_crd.py @@ -472,8 +472,8 @@ class TestGerritToGithubCRD(ZuulTestCase): self.assertEqual(B.reported, 1) # Update A to add A->B (a cycle). - A.editBody('Depends-On: %s\n' % (B.data['url'])) - self.fake_github.emitEvent(A.getPullRequestEditedEvent()) + self.fake_github.emitEvent( + A.editBody('Depends-On: %s\n' % (B.data['url']))) self.waitUntilSettled() # Dependency cycle injected so zuul should have reported again on A @@ -701,9 +701,8 @@ class TestGithubToGerritCRD(ZuulTestCase): 'gerrit/project1', 'master', 'B') # A Depends-On: B - A.editBody('Depends-On: %s\n' % (B.data['url'],)) - - self.fake_github.emitEvent(A.getPullRequestEditedEvent()) + self.fake_github.emitEvent( + A.editBody('Depends-On: %s\n' % (B.data['url'],))) self.waitUntilSettled() self.hold_jobs_in_queue = False @@ -741,12 +740,12 @@ class TestGithubToGerritCRD(ZuulTestCase): 'gerrit/project1', 'master', 'B') # A Depends-On: B - A.editBody('Depends-On: %s\n' % (B.data['url'],)) + event = A.editBody('Depends-On: %s\n' % (B.data['url'],)) tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') check_pipeline = tenant.layout.pipelines['check'] # Add two dependent changes... - self.fake_github.emitEvent(A.getPullRequestEditedEvent()) + self.fake_github.emitEvent(event) self.waitUntilSettled() self.assertEqual(len(check_pipeline.getAllItems()), 2) @@ -794,9 +793,8 @@ class TestGithubToGerritCRD(ZuulTestCase): 'gerrit/project1', 'master', 'B') # A Depends-On: B - A.editBody('Depends-On: %s\n' % (B.data['url'],)) - - self.fake_github.emitEvent(A.getPullRequestEditedEvent()) + self.fake_github.emitEvent( + A.editBody('Depends-On: %s\n' % (B.data['url'],))) self.waitUntilSettled() self.scheds.execute(lambda app: app.sched.reconfigure(app.config)) @@ -856,9 +854,8 @@ class TestGithubToGerritCRD(ZuulTestCase): B.subject, C.url) # A Depends-On: B - A.editBody('Depends-On: %s\n' % (B.data['url'],)) - - self.fake_github.emitEvent(A.getPullRequestEditedEvent()) + self.fake_github.emitEvent( + A.editBody('Depends-On: %s\n' % (B.data['url'],))) self.waitUntilSettled() self.assertEqual(self.history[-1].changes, '2,%s 1,1 1,%s' % (C.head_sha, A.head_sha)) @@ -900,14 +897,14 @@ class TestGithubToGerritCRD(ZuulTestCase): 'gerrit/unknown', 'master', 'B') # A Depends-On: B - A.editBody('Depends-On: %s\n' % (B.data['url'],)) + event = A.editBody('Depends-On: %s\n' % (B.data['url'],)) # Make sure zuul has seen an event on B. self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) # Note we wait until settled here as the event processing for # the next event may not have the updated db yet otherwise. self.waitUntilSettled() - self.fake_github.emitEvent(A.getPullRequestEditedEvent()) + self.fake_github.emitEvent(event) self.waitUntilSettled() self.assertFalse(A.is_merged) @@ -927,8 +924,8 @@ class TestGithubToGerritCRD(ZuulTestCase): # Create B->A B = self.fake_github.openFakePullRequest('github/project2', 'master', 'B') - B.editBody('Depends-On: %s\n' % (A.data['url'],)) - self.fake_github.emitEvent(B.getPullRequestEditedEvent()) + self.fake_github.emitEvent( + B.editBody('Depends-On: %s\n' % (A.data['url'],))) self.waitUntilSettled() # Dep is there so zuul should have reported on B diff --git a/tests/unit/test_github_crd.py b/tests/unit/test_github_crd.py index 5280ba25a4..614ffb63cf 100644 --- a/tests/unit/test_github_crd.py +++ b/tests/unit/test_github_crd.py @@ -204,8 +204,7 @@ class TestGithubCrossRepoDeps(ZuulTestCase): # Update B to point at A1 instead of A msg = "Depends-On: https://github.com/org/project1/pull/%s" % A1.number - B.editBody(msg) - self.fake_github.emitEvent(B.getPullRequestEditedEvent()) + self.fake_github.emitEvent(B.editBody(msg)) self.waitUntilSettled() # Release diff --git a/tests/unit/test_pagure_driver.py b/tests/unit/test_pagure_driver.py index 033574f30e..6956bf6100 100644 --- a/tests/unit/test_pagure_driver.py +++ b/tests/unit/test_pagure_driver.py @@ -1019,11 +1019,8 @@ class TestGithubToPagureCRD(ZuulTestCase): 'pagure/project2', 'master', 'B') # A Depends-On: B - A.editBody('Depends-On: %s\n' % B.url) - self.executor_server.hold_jobs_in_build = True - - self.fake_github.emitEvent(A.getPullRequestEditedEvent()) + self.fake_github.emitEvent(A.editBody('Depends-On: %s\n' % B.url)) self.waitUntilSettled() self.assertTrue(self.builds[0].hasChanges(A, B)) diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 8307990aa1..334531cad6 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -463,6 +463,8 @@ class GithubEventProcessor(object): event.label = self.body['label']['name'] elif action == 'edited': event.action = 'edited' + if 'body' in self.body.get('changes', {}): + event.body_edited = True else: return None diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py index 6db5df83b9..9b9e529143 100644 --- a/zuul/driver/github/githubmodel.py +++ b/zuul/driver/github/githubmodel.py @@ -107,6 +107,7 @@ class GithubTriggerEvent(TriggerEvent): self.check_run = None self.status = None self.commits = [] + self.body_edited = None def toDict(self): d = super().toDict() @@ -118,6 +119,7 @@ class GithubTriggerEvent(TriggerEvent): d["check_run"] = self.check_run d["status"] = self.status d["commits"] = self.commits + d["body_edited"] = self.body_edited return d def updateFromDict(self, d): @@ -130,12 +132,16 @@ class GithubTriggerEvent(TriggerEvent): self.check_run = d["check_run"] self.status = d["status"] self.commits = d["commits"] + self.body_edited = d["body_edited"] def isPatchsetCreated(self): if self.type == 'pull_request': return self.action in ['opened', 'changed'] return False + def isMessageChanged(self): + return bool(self.body_edited) + def isChangeAbandoned(self): if self.type == 'pull_request': return 'closed' == self.action diff --git a/zuul/driver/gitlab/gitlabconnection.py b/zuul/driver/gitlab/gitlabconnection.py index f5da2f8f04..5c5675ad69 100644 --- a/zuul/driver/gitlab/gitlabconnection.py +++ b/zuul/driver/gitlab/gitlabconnection.py @@ -124,6 +124,8 @@ class GitlabEventConnector(threading.Thread): event.patch_number = attrs['last_commit']['id'] event.change_url = self.connection.getMRUrl(event.project_name, event.change_number) + if attrs['action'] == 'update' and "description" in body["changes"]: + event.merge_request_description_changed = True if attrs['action'] == 'open': event.action = 'opened' elif attrs['action'] == 'merge': diff --git a/zuul/driver/gitlab/gitlabmodel.py b/zuul/driver/gitlab/gitlabmodel.py index 2933c8a25b..9ec8926284 100644 --- a/zuul/driver/gitlab/gitlabmodel.py +++ b/zuul/driver/gitlab/gitlabmodel.py @@ -78,6 +78,7 @@ class GitlabTriggerEvent(TriggerEvent): self.action = None self.labels = [] self.change_number = None + self.merge_request_description_changed = None self.tag = None def toDict(self): @@ -87,6 +88,8 @@ class GitlabTriggerEvent(TriggerEvent): d["action"] = self.action d["labels"] = self.labels d["change_number"] = self.change_number + d["merge_request_description_changed"] = \ + self.merge_request_description_changed d["tag"] = self.tag return d @@ -97,6 +100,8 @@ class GitlabTriggerEvent(TriggerEvent): self.action = d["action"] self.labels = d["labels"] self.change_number = d["change_number"] + self.merge_request_description_changed = \ + d["merge_request_description_changed"] self.tag = d["tag"] def _repr(self): @@ -115,6 +120,9 @@ class GitlabTriggerEvent(TriggerEvent): return self.action in ['opened', 'changed'] return False + def isMessageChanged(self): + return bool(self.merge_request_description_changed) + class GitlabEventFilter(EventFilter): def __init__( diff --git a/zuul/driver/pagure/pagureconnection.py b/zuul/driver/pagure/pagureconnection.py index d75b0af231..4de5435ca1 100644 --- a/zuul/driver/pagure/pagureconnection.py +++ b/zuul/driver/pagure/pagureconnection.py @@ -242,6 +242,7 @@ class PagureEventConnector(threading.Thread): """ Handles pull request initial comment change """ event, _ = self._event_base(body) event.action = 'changed' + event.initial_comment_changed = True return event def _event_pull_request_tags_changed(self, body): diff --git a/zuul/driver/pagure/paguremodel.py b/zuul/driver/pagure/paguremodel.py index 052f8e3b98..00d5b14022 100644 --- a/zuul/driver/pagure/paguremodel.py +++ b/zuul/driver/pagure/paguremodel.py @@ -87,6 +87,7 @@ class PagureTriggerEvent(TriggerEvent): self.trigger_name = 'pagure' self.title = None self.action = None + self.initial_comment_changed = None self.status = None self.tags = [] self.tag = None @@ -96,6 +97,7 @@ class PagureTriggerEvent(TriggerEvent): d["trigger_name"] = self.trigger_name d["title"] = self.title d["action"] = self.action + d["initial_comment_changed"] = self.initial_comment_changed d["status"] = self.status d["tags"] = list(self.tags) d["tag"] = self.tag @@ -106,6 +108,7 @@ class PagureTriggerEvent(TriggerEvent): self.trigger_name = d.get("trigger_name", "pagure") self.title = d.get("title") self.action = d.get("action") + self.initial_comment_changed = d.get("initial_comment_changed") self.status = d.get("status") self.tags = d.get("tags", []) self.tag = d.get("tag") @@ -128,6 +131,9 @@ class PagureTriggerEvent(TriggerEvent): return self.action in ['opened', 'changed'] return False + def isMessageChanged(self): + return bool(self.initial_comment_changed) + class PagureEventFilter(EventFilter): def __init__(self, connection_name, trigger, types=[], refs=[], diff --git a/zuul/model.py b/zuul/model.py index 8cec5aa0d2..6b35204d4e 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -4422,6 +4422,9 @@ class TriggerEvent(AbstractEvent): def isPatchsetCreated(self): return False + def isMessageChanged(self): + return False + def isChangeAbandoned(self): return False diff --git a/zuul/scheduler.py b/zuul/scheduler.py index f7e54612ba..b52a684847 100644 --- a/zuul/scheduler.py +++ b/zuul/scheduler.py @@ -1750,11 +1750,8 @@ class Scheduler(threading.Thread): # Let the pipeline update any dependencies that may need # refreshing if this change has updated. - - # TODO: We only really need to run this if we have a new - # patchest (isPatchsetCreated()) or the depends-on list in the - # PR body has changed (but we don't have a way to test that yet). - pipeline.manager.refreshDeps(change, event) + if event.isPatchsetCreated() or event.isMessageChanged(): + pipeline.manager.refreshDeps(change, event) if event.isPatchsetCreated(): pipeline.manager.removeOldVersionsOfChange(change, event)