diff --git a/tests/fixtures/layouts/submitted-together-per-branch.yaml b/tests/fixtures/layouts/submitted-together-per-branch.yaml new file mode 100644 index 0000000000..73898ac994 --- /dev/null +++ b/tests/fixtures/layouts/submitted-together-per-branch.yaml @@ -0,0 +1,70 @@ +- queue: + name: integrated + allow-circular-dependencies: true + per-branch: true + +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + +- pipeline: + name: gate + manager: dependent + success-message: Build succeeded (gate). + require: + gerrit: + approval: + - Approved: 1 + trigger: + gerrit: + - event: comment-added + approval: + - Approved: 1 + success: + gerrit: + Verified: 2 + submit: true + failure: + gerrit: + Verified: -2 + start: + gerrit: + Verified: 0 + precedence: high + +- job: + name: base + parent: null + run: playbooks/run.yaml + +- job: + name: test-job + +- project: + name: org/project1 + queue: integrated + check: + jobs: + - test-job + gate: + jobs: + - test-job + +- project: + name: org/project2 + queue: integrated + check: + jobs: + - test-job + gate: + jobs: + - test-job diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py index 713408ee98..f26aaeb96f 100644 --- a/tests/unit/test_circular_dependencies.py +++ b/tests/unit/test_circular_dependencies.py @@ -17,7 +17,7 @@ import textwrap from zuul.model import PromoteEvent -from tests.base import ZuulTestCase +from tests.base import ZuulTestCase, simple_layout class TestGerritCircularDependencies(ZuulTestCase): @@ -180,7 +180,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 1) + self.assertEqual(A.reported, 2) self.assertEqual(B.reported, 1) self.assertEqual(A.data["status"], "NEW") self.assertEqual(B.data["status"], "NEW") @@ -1266,7 +1266,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.assertEqual(A.reported, 3) self.assertEqual(B.reported, 3) - self.assertEqual(C.reported, 3) + self.assertEqual(C.reported, 6) self.assertEqual(A.data["status"], "MERGED") self.assertEqual(B.data["status"], "MERGED") self.assertEqual(C.data["status"], "MERGED") @@ -1300,7 +1300,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 1) + self.assertEqual(A.reported, 2) self.assertEqual(A.data["status"], "NEW") self.assertEqual(B.data["status"], "NEW") @@ -1311,7 +1311,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 3) + self.assertEqual(A.reported, 4) self.assertEqual(A.data["status"], "MERGED") def test_promote_cycle(self): @@ -1449,6 +1449,27 @@ class TestGerritCircularDependencies(ZuulTestCase): self.assertEqual(A.data["status"], "MERGED") self.assertEqual(B.data["status"], "MERGED") + @simple_layout('layouts/submitted-together-per-branch.yaml') + def test_submitted_together_per_branch(self): + self.fake_gerrit._fake_submit_whole_topic = True + self.create_branch('org/project2', 'stable/foo') + A = self.fake_gerrit.addFakeChange('org/project1', "master", "A", + topic='test-topic') + B = self.fake_gerrit.addFakeChange('org/project2', "stable/foo", "B", + topic='test-topic') + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + A.addApproval("Approved", 1) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 0) + self.assertEqual(B.reported, 1) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + self.assertIn("does not share a change queue", B.messages[-1]) + class TestGithubCircularDependencies(ZuulTestCase): config_file = "zuul-gerrit-github.conf" diff --git a/tests/unit/test_cross_crd.py b/tests/unit/test_cross_crd.py index 442277f51d..42d86275ba 100644 --- a/tests/unit/test_cross_crd.py +++ b/tests/unit/test_cross_crd.py @@ -669,7 +669,7 @@ class TestGithubToGerritCRD(ZuulTestCase): # should not be processed in dependent pipeline self.assertFalse(A.is_merged) self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(len(A.comments), 0) + self.assertEqual(len(A.comments), 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -688,7 +688,7 @@ class TestGithubToGerritCRD(ZuulTestCase): self.waitUntilSettled() self.assertTrue(A.is_merged) - self.assertEqual(len(A.comments), 2) + self.assertEqual(len(A.comments), 3) self.assertEqual(B.data['status'], 'MERGED') self.assertEqual(B.reported, 0) diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py index d925666845..0b0ca86feb 100644 --- a/tests/unit/test_gerrit_crd.py +++ b/tests/unit/test_gerrit_crd.py @@ -253,7 +253,7 @@ class TestGerritCRD(ZuulTestCase): self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -270,7 +270,7 @@ class TestGerritCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) def _test_crd_gate_reverse(self, url_fmt): "Test reverse cross-repo dependencies" @@ -369,7 +369,7 @@ class TestGerritCRD(ZuulTestCase): # should not be processed in dependent pipeline self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -388,7 +388,7 @@ class TestGerritCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) self.assertEqual(B.data['status'], 'MERGED') self.assertEqual(B.reported, 0) diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py index 5c3e868f93..60616b6cff 100644 --- a/tests/unit/test_gerrit_legacy_crd.py +++ b/tests/unit/test_gerrit_legacy_crd.py @@ -194,7 +194,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -211,7 +211,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) def test_crd_gate_reverse(self): "Test reverse cross-repo dependencies" @@ -298,7 +298,7 @@ class TestGerritLegacyCRD(ZuulTestCase): # should not be processed in dependent pipeline self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -317,7 +317,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) self.assertEqual(B.data['status'], 'MERGED') self.assertEqual(B.reported, 0) diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 0c8f2ddc37..63614b4c45 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -353,7 +353,8 @@ class PipelineManager(metaclass=ABCMeta): return True def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None, dependency_graph=None): + change_queue, history=None, dependency_graph=None, + warnings=None): return True def enqueueChangesBehind(self, change, event, quiet, ignore_requirements, @@ -525,13 +526,18 @@ class PipelineManager(metaclass=ABCMeta): (change, change.project)) return False + warnings = [] if not self.enqueueChangesAhead(change, event, quiet, ignore_requirements, change_queue, history=history, - dependency_graph=dependency_graph): + dependency_graph=dependency_graph, + warnings=warnings): self.dequeueIncompleteCycle(change, dependency_graph, event, change_queue) log.debug("Failed to enqueue changes ahead of %s" % change) + if warnings: + self._reportNonEqueuedItem(change_queue, change, + event, warnings) return False log.debug("History after enqueuing changes ahead: %s", history) @@ -546,22 +552,9 @@ class PipelineManager(metaclass=ABCMeta): if cycle and not self.canProcessCycle(change.project): log.info("Dequeing change %s since at least one project " "does not allow circular dependencies", change) - actions = self.pipeline.failure_actions - ci = change_queue.enqueueChange(cycle[-1], event) - ci.warning("Dependency cycle detected") - ci.setReportedResult('FAILURE') - - # Only report the cycle if the project is in the current - # pipeline. Otherwise the change could be spammed by - # reports from unrelated pipelines. - if self.pipeline.tenant.layout.getProjectPipelineConfig( - ci - ): - self.sendReport(actions, ci) - self.dequeueItem(ci) - self.sql.reportBuildsetEnd(ci.current_build_set, - 'failure', final=True) - + warnings = ["Dependency cycle detected"] + self._reportNonEqueuedItem(change_queue, + cycle[-1], event, warnings) return False log.info("Adding change %s to queue %s in %s" % @@ -600,6 +593,24 @@ class PipelineManager(metaclass=ABCMeta): self.dequeueSupercededItems(item) return True + def _reportNonEqueuedItem(self, change_queue, change, event, warnings): + # Enqueue an item which otherwise can not be enqueued in order + # to report a message to the user. + actions = self.pipeline.failure_actions + ci = change_queue.enqueueChange(change, event) + for w in warnings: + ci.warning(w) + ci.setReportedResult('FAILURE') + + # Only report the item if the project is in the current + # pipeline. Otherwise the change could be spammed by + # reports from unrelated pipelines. + if self.pipeline.tenant.layout.getProjectPipelineConfig(ci): + self.sendReport(actions, ci) + self.dequeueItem(ci) + self.sql.reportBuildsetEnd(ci.current_build_set, + 'failure', final=True) + def cycleForChange(self, change, dependency_graph, event): log = get_annotated_logger(self.log, event) log.debug("Running Tarjan's algorithm on current dependencies: %s", diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index 30b7c97bcb..d4d4f05dd0 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -138,7 +138,8 @@ class DependentPipelineManager(SharedQueuePipelineManager): dependency_graph=dependency_graph) def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None, dependency_graph=None): + change_queue, history=None, dependency_graph=None, + warnings=None): log = get_annotated_logger(self.log, event) history = history if history is not None else [] @@ -149,7 +150,8 @@ class DependentPipelineManager(SharedQueuePipelineManager): return True ret = self.checkForChangesNeededBy(change, change_queue, event, - dependency_graph=dependency_graph) + dependency_graph=dependency_graph, + warnings=warnings) if ret in [True, False]: return ret log.debug(" Changes %s must be merged ahead of %s", ret, change) @@ -168,7 +170,7 @@ class DependentPipelineManager(SharedQueuePipelineManager): return True def checkForChangesNeededBy(self, change, change_queue, event, - dependency_graph=None): + dependency_graph=None, warnings=None): log = get_annotated_logger(self.log, event) # Return true if okay to proceed enqueing this change, @@ -200,11 +202,16 @@ class DependentPipelineManager(SharedQueuePipelineManager): with self.getChangeQueue(needed_change, event) as needed_change_queue: if needed_change_queue != change_queue: - log.debug(" Change %s in project %s does not " - "share a change queue with %s " - "in project %s", - needed_change, needed_change.project, - change, change.project) + msg = ("Change %s in project %s does not " + "share a change queue with %s " + "in project %s" % + (needed_change.number, + needed_change.project, + change.number, + change.project)) + log.debug(" " + msg) + if warnings is not None: + warnings.append(msg) return False if not needed_change.is_current_patchset: log.debug(" Needed change is not the current patchset") diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index 4405c44817..7cd14ffd56 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -38,7 +38,8 @@ class IndependentPipelineManager(PipelineManager): return DynamicChangeQueueContextManager(change_queue) def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None, dependency_graph=None): + change_queue, history=None, dependency_graph=None, + warnings=None): log = get_annotated_logger(self.log, event) history = history if history is not None else []