From c2a33fcb9c020454a73549a1d4fcb383599867eb Mon Sep 17 00:00:00 2001 From: Fabien Boucher Date: Wed, 24 Jul 2019 12:12:51 +0200 Subject: [PATCH] Return dependency cycle failure to user This change changes the dependency cycle error handling in order to report the error to the user via the standard report mechanics. Change-Id: I7cdbd9b7b48fd612b2887007b70acedd7a8e3113 --- tests/unit/test_cross_crd.py | 19 +++++++++++-------- tests/unit/test_gerrit_crd.py | 8 ++++---- tests/unit/test_gerrit_legacy_crd.py | 8 ++++---- zuul/manager/__init__.py | 11 +++++++++++ zuul/manager/dependent.py | 4 ---- zuul/manager/independent.py | 4 ---- 6 files changed, 30 insertions(+), 24 deletions(-) diff --git a/tests/unit/test_cross_crd.py b/tests/unit/test_cross_crd.py index 74e64a6894..e82e43688e 100644 --- a/tests/unit/test_cross_crd.py +++ b/tests/unit/test_cross_crd.py @@ -183,7 +183,10 @@ class TestGerritToGithubCRD(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval('Approved', 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) + self.assertEqual( + A.messages[0], + "Build failed.\n\n\nWarning:\n Dependency cycle detected\n") self.assertEqual(len(B.comments), 0) self.assertEqual(A.data['status'], 'NEW') self.assertFalse(B.is_merged) @@ -469,8 +472,8 @@ class TestGerritToGithubCRD(ZuulTestCase): self.fake_github.emitEvent(A.getPullRequestEditedEvent()) self.waitUntilSettled() - # Dependency cycle injected so zuul should not have reported again on A - self.assertEqual(len(A.comments), 1) + # Dependency cycle injected so zuul should have reported again on A + self.assertEqual(len(A.comments), 2) # Now if we update B to remove the depends-on, everything # should be okay. B; A->B @@ -481,7 +484,7 @@ class TestGerritToGithubCRD(ZuulTestCase): self.waitUntilSettled() # Cycle was removed so now zuul should have reported again on A - self.assertEqual(len(A.comments), 2) + self.assertEqual(len(A.comments), 3) self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(2)) self.waitUntilSettled() @@ -634,7 +637,7 @@ class TestGithubToGerritCRD(ZuulTestCase): self.fake_github.emitEvent(A.addLabel('approved')) self.waitUntilSettled() - self.assertEqual(len(A.comments), 0) + self.assertEqual(len(A.comments), 1) self.assertEqual(B.reported, 0) self.assertFalse(A.is_merged) self.assertEqual(B.data['status'], 'NEW') @@ -930,8 +933,8 @@ class TestGithubToGerritCRD(ZuulTestCase): self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) self.waitUntilSettled() - # Dependency cycle injected so zuul should not have reported again on A - self.assertEqual(A.reported, 1) + # Dependency cycle injected so zuul should have reported again on A + self.assertEqual(A.reported, 2) # Now if we update B to remove the depends-on, everything # should be okay. B; A->B @@ -942,7 +945,7 @@ class TestGithubToGerritCRD(ZuulTestCase): self.waitUntilSettled() # Cycle was removed so now zuul should have reported again on A - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) self.fake_github.emitEvent(B.getPullRequestEditedEvent()) self.waitUntilSettled() diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py index 5f796ac057..3edb9de6a8 100644 --- a/tests/unit/test_gerrit_crd.py +++ b/tests/unit/test_gerrit_crd.py @@ -335,7 +335,7 @@ class TestGerritCRD(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval('Approved', 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') @@ -698,8 +698,8 @@ class TestGerritCRD(ZuulTestCase): self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(2)) self.waitUntilSettled() - # Dependency cycle injected so zuul should not have reported again on A - self.assertEqual(A.reported, 1) + # Dependency cycle injected so zuul should have reported again on A + self.assertEqual(A.reported, 2) # Now if we update B to remove the depends-on, everything # should be okay. B; A->B @@ -710,7 +710,7 @@ class TestGerritCRD(ZuulTestCase): self.waitUntilSettled() # Cycle was removed so now zuul should have reported again on A - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(2)) self.waitUntilSettled() diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py index 0bed2236fb..515e2fa95d 100644 --- a/tests/unit/test_gerrit_legacy_crd.py +++ b/tests/unit/test_gerrit_legacy_crd.py @@ -270,7 +270,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval('Approved', 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') @@ -609,8 +609,8 @@ class TestGerritLegacyCRD(ZuulTestCase): self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(2)) self.waitUntilSettled() - # Dependency cycle injected so zuul should not have reported again on A - self.assertEqual(A.reported, 1) + # Dependency cycle injected so zuul should have reported again on A + self.assertEqual(A.reported, 2) # Now if we update B to remove the depends-on, everything # should be okay. B; A->B @@ -621,7 +621,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.waitUntilSettled() # Cycle was removed so now zuul should have reported again on A - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(2)) self.waitUntilSettled() diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index ea06819545..af3547320b 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -310,6 +310,17 @@ class PipelineManager(object): (change, change.project)) return False + if history and change in history: + log.debug("Dependency cycle detected for " + "change %s in project %s" % ( + change, change.project)) + item = model.QueueItem(self, change, event) + item.warning("Dependency cycle detected") + actions = self.pipeline.failure_actions + item.setReportedResult('FAILURE') + self.sendReport(actions, item) + return False + if not self.enqueueChangesAhead(change, event, quiet, ignore_requirements, change_queue, history=history): diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index 2f47013cf5..17a403dd14 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -161,10 +161,6 @@ class DependentPipelineManager(PipelineManager): change_queue, history=None): log = get_annotated_logger(self.log, event) - if history and change in history: - # detected dependency cycle - log.warn("Dependency cycle detected") - return False if hasattr(change, 'number'): history = history or [] history = history + [change] diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index 1ff0f52ca0..682020a029 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -40,10 +40,6 @@ class IndependentPipelineManager(PipelineManager): change_queue, history=None): log = get_annotated_logger(self.log, event) - if history and change in history: - # detected dependency cycle - log.warn("Dependency cycle detected") - return False if hasattr(change, 'number'): history = history or [] history = history + [change]