From 98ad228bccfbbf4e13797b2a63ea27b4f98dd0a6 Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Thu, 23 Jan 2020 13:23:53 +0100 Subject: [PATCH] Fix mqtt log url reporting when report-build-page is active Currently the mqtt reporter uses the report url as log_url. This is fine as long as report-build-page is disabled. As soon as report-build-page is enabled on a tenant it reports the url to the result page of the build. As mqtt is meant to be consumed by machines this breaks e.g. log post processing. Fix this by reporting the real log url as log_url and add the field web_url for use cases where really the human url is required. This fixes also a wrong indentation in the mqtt driver documentation, resulting in all buildset.builds.* attributes being listed as buildset.* attributes. Change-Id: I91ce93a7000ddd0d70ce504b70742262d8239a8f --- doc/source/reference/drivers/mqtt.rst | 9 +++- ...tt-report-build-page-1780225ca98c7456.yaml | 13 +++++ .../git/common-config/playbooks/test.yaml | 2 + .../git/common-config/zuul.d/config.yaml | 45 ++++++++++++++++ .../git/org_project/README | 1 + .../mqtt-driver-report-build-page/main.yaml | 9 ++++ tests/fixtures/zuul-mqtt-driver.conf | 3 ++ tests/unit/test_connection.py | 52 +++++++++++++++++++ zuul/driver/mqtt/mqttreporter.py | 5 +- zuul/driver/sql/sqlreporter.py | 7 +-- zuul/model.py | 7 +++ 11 files changed, 145 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/mqtt-report-build-page-1780225ca98c7456.yaml create mode 100644 tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/playbooks/test.yaml create mode 100644 tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/zuul.d/config.yaml create mode 100644 tests/fixtures/config/mqtt-driver-report-build-page/git/org_project/README create mode 100644 tests/fixtures/config/mqtt-driver-report-build-page/main.yaml diff --git a/doc/source/reference/drivers/mqtt.rst b/doc/source/reference/drivers/mqtt.rst index b4a7068d04..8aa0911dc2 100644 --- a/doc/source/reference/drivers/mqtt.rst +++ b/doc/source/reference/drivers/mqtt.rst @@ -72,7 +72,7 @@ An MQTT report uses this schema: .. attr:: builds - The list of builds. + The list of builds. .. attr:: job_name @@ -102,6 +102,12 @@ An MQTT report uses this schema: The build log url (not present in start report). + .. attr:: web_url + + The url to the build result page if :attr:`tenant.report-build-page` + is enabled. The build log url otherwise (not present in start + report). + .. attr:: result The build results (not present in start report). @@ -163,6 +169,7 @@ Here is an example of a success message: 'start_time': 1524801179.8557224, 'end_time': 1524801208.928095, 'log_url': 'https://logs.example.com/logs/3/3/1/check/linters/16e3e55/', + 'web_url': 'https://tenant.example.com/t/tenant-one/build/16e3e55aca984c6c9a50cc3c5b21bb83/', 'result': 'SUCCESS', 'dependencies': [] }], diff --git a/releasenotes/notes/mqtt-report-build-page-1780225ca98c7456.yaml b/releasenotes/notes/mqtt-report-build-page-1780225ca98c7456.yaml new file mode 100644 index 0000000000..34b1a7348b --- /dev/null +++ b/releasenotes/notes/mqtt-report-build-page-1780225ca98c7456.yaml @@ -0,0 +1,13 @@ +--- +fixes: + - | + Although the documentation states that the MQTT reporter reports the build's + log_url, this was only true as long as :attr:`tenant.report-build-page` was + disabled. As soon as the setting was enabled, the MQTT reporter reported the + url to the build's result page in Zuul. As MQTT is meant to be consumed by + machines, this broke use cases like log post processing. + + This was fixed so that the :attr:`.buildset.builds.log_url` now + always contains the log url, while an additional field + :attr:`.buildset.builds.web_url` contains the url to the + build's result page if :attr:`tenant.report-build-page` is enabled. diff --git a/tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/playbooks/test.yaml b/tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/playbooks/test.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/playbooks/test.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/zuul.d/config.yaml b/tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/zuul.d/config.yaml new file mode 100644 index 0000000000..c842e94246 --- /dev/null +++ b/tests/fixtures/config/mqtt-driver-report-build-page/git/common-config/zuul.d/config.yaml @@ -0,0 +1,45 @@ +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + start: + mqtt: + topic: "{tenant}/zuul_start/{pipeline}/{project}/{branch}" + success: + gerrit: + Verified: 1 + mqtt: + topic: "{tenant}/zuul_buildset/{pipeline}/{project}/{branch}" + failure: + gerrit: + Verified: -1 + mqtt: + topic: "{tenant}/zuul_buildset/{pipeline}/{project}/{branch}" + +- job: + name: base + parent: null + +- job: + name: test + run: playbooks/test.yaml + +- job: + name: dependent-test + run: playbooks/test.yaml + +- project: + name: org/project + check: + jobs: + - test + - dependent-test: + dependencies: + - test + +- project: + name: common-config + check: + jobs: [] diff --git a/tests/fixtures/config/mqtt-driver-report-build-page/git/org_project/README b/tests/fixtures/config/mqtt-driver-report-build-page/git/org_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/mqtt-driver-report-build-page/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/mqtt-driver-report-build-page/main.yaml b/tests/fixtures/config/mqtt-driver-report-build-page/main.yaml new file mode 100644 index 0000000000..2a5a4f4cc3 --- /dev/null +++ b/tests/fixtures/config/mqtt-driver-report-build-page/main.yaml @@ -0,0 +1,9 @@ +- tenant: + name: tenant-one + report-build-page: true + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project diff --git a/tests/fixtures/zuul-mqtt-driver.conf b/tests/fixtures/zuul-mqtt-driver.conf index 45d31827f3..f25ea41a42 100644 --- a/tests/fixtures/zuul-mqtt-driver.conf +++ b/tests/fixtures/zuul-mqtt-driver.conf @@ -20,3 +20,6 @@ sshkey=fake_id_rsa1 [connection mqtt] driver=mqtt + +[web] +root=https://tenant.example.com/ diff --git a/tests/unit/test_connection.py b/tests/unit/test_connection.py index 4f15d888db..5e492eabad 100644 --- a/tests/unit/test_connection.py +++ b/tests/unit/test_connection.py @@ -542,6 +542,9 @@ class TestMQTTConnection(ZuulTestCase): "Test the MQTT reporter" # Add a success result A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + self.executor_server.returnData( + "test", A, {"zuul": {"log_url": "some-log-url"}} + ) self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) self.waitUntilSettled() @@ -572,6 +575,10 @@ class TestMQTTConnection(ZuulTestCase): self.assertEquals(test_job['job_name'], 'test') self.assertEquals(test_job['result'], 'SUCCESS') self.assertEquals(test_job['dependencies'], []) + # Both log- and web-url should point to the same URL which is specified + # in the build result data under zuul.log_url. + self.assertEquals(test_job['log_url'], 'some-log-url/') + self.assertEquals(test_job['web_url'], 'some-log-url') self.assertIn('execute_time', test_job) self.assertIn('timestamp', mqtt_payload) self.assertIn('enqueue_time', mqtt_payload) @@ -600,3 +607,48 @@ class TestMQTTConnection(ZuulTestCase): self.assertIn("topic component 'bad' is invalid", A.messages[0], "A should report a syntax error") + + +class TestMQTTConnectionBuildPage(ZuulTestCase): + config_file = "zuul-mqtt-driver.conf" + tenant_config_file = "config/mqtt-driver-report-build-page/main.yaml" + + def test_mqtt_reporter(self): + "Test the MQTT reporter with 'report-build-page' enabled" + + # Add a sucess result + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + self.executor_server.returnData( + "test", A, {"zuul": {"log_url": "some-log-url"}} + ) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + success_event = self.mqtt_messages.pop() + + self.assertEquals( + success_event.get("topic"), + "tenant-one/zuul_buildset/check/org/project/master", + ) + + mqtt_payload = success_event["msg"] + self.assertEquals(mqtt_payload["project"], "org/project") + self.assertEquals(mqtt_payload["branch"], "master") + self.assertEquals(mqtt_payload["buildset"]["result"], "SUCCESS") + + builds = mqtt_payload["buildset"]["builds"] + test_job = [b for b in builds if b["job_name"] == "test"][0] + self.assertEquals(test_job["job_name"], "test") + self.assertEquals(test_job["result"], "SUCCESS") + + build_id = test_job["uuid"] + # When report-build-page is enabled, the log_url should still point to + # the URL that is specified in the build result data under + # zuul.log_url. The web_url will instead point to the builds page. + self.assertEquals(test_job["log_url"], "some-log-url/") + self.assertEquals( + test_job["web_url"], + "https://tenant.example.com/t/tenant-one/build/{}".format( + build_id + ), + ) diff --git a/zuul/driver/mqtt/mqttreporter.py b/zuul/driver/mqtt/mqttreporter.py index 74b597feaa..88dd70738d 100644 --- a/zuul/driver/mqtt/mqttreporter.py +++ b/zuul/driver/mqtt/mqttreporter.py @@ -61,13 +61,14 @@ class MQTTReporter(BaseReporter): build = item.current_build_set.getBuild(job.name) if build: # Report build data if available - (result, url) = item.formatJobResult(job) + (result, web_url) = item.formatJobResult(job) job_informations.update({ 'uuid': build.uuid, 'start_time': build.start_time, 'end_time': build.end_time, 'execute_time': build.execute_time, - 'log_url': url, + 'log_url': build.log_url, + 'web_url': web_url, 'result': result, 'dependencies': [j.name for j in job.dependencies], }) diff --git a/zuul/driver/sql/sqlreporter.py b/zuul/driver/sql/sqlreporter.py index 1c4caa891e..b77f9e6cf2 100644 --- a/zuul/driver/sql/sqlreporter.py +++ b/zuul/driver/sql/sqlreporter.py @@ -30,10 +30,7 @@ class SQLReporter(BaseReporter): log = logging.getLogger("zuul.SQLReporter") def _getBuildData(self, item, job, build): - (result, url) = item.formatJobResult(job, build) - log_url = build.result_data.get('zuul', {}).get('log_url') - if log_url and log_url[-1] != '/': - log_url = log_url + '/' + (result, _) = item.formatJobResult(job, build) start = end = None if build.start_time: start = datetime.datetime.fromtimestamp( @@ -43,7 +40,7 @@ class SQLReporter(BaseReporter): end = datetime.datetime.fromtimestamp( build.end_time, tz=datetime.timezone.utc) - return result, log_url, start, end + return result, build.log_url, start, end def createBuildEntry(self, item, job, db_buildset, build, final=True): # Ensure end_time is defined diff --git a/zuul/model.py b/zuul/model.py index ccfea354eb..e343765b18 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -1906,6 +1906,13 @@ class Build(object): def pipeline(self): return self.build_set.item.pipeline + @property + def log_url(self): + log_url = self.result_data.get('zuul', {}).get('log_url') + if log_url and log_url[-1] != '/': + log_url = log_url + '/' + return log_url + def getSafeAttributes(self): return Attributes(uuid=self.uuid, result=self.result,