fix(reporters): restore old behavior for Gerrit reporting

This is an attempt to restore the old formatting, e.g. with failed
checks and a link to the URI.

At the same time, this attempts to fix the eager +1 Verified tag which
is sent when nix-eval is started (?) and not done or when the evaluation
is done instead of the whole nix-eval job seen as completed.

Signed-off-by: Raito Bezarius <raito@lix.systems>

buildbot_nix/__init__.py

@@ -14,6 +14,7 @@ import tempfile
 import buildbot
 from buildbot.configurators import ConfiguratorBase
 from buildbot.plugins import reporters, schedulers, secrets, steps, util, worker
+from buildbot.plugins.util import BuilderConfig, MasterLock
 from buildbot.process import buildstep, logobserver, remotecommand
 from buildbot.process.project import Project
 from buildbot.process.properties import Properties
@@ -22,7 +23,7 @@ from buildbot.www.auth import AuthBase
 from buildbot.www.oauth2 import OAuth2Auth
 from buildbot.changes.gerritchangesource import GerritChangeSource
 from buildbot.reporters.utils import getURLForBuildrequest
-from buildbot.reporters.generators.build import BuildStatusGenerator
+from buildbot.reporters.gerrit import GerritBuildEndStatusGenerator
 from buildbot.reporters.message import MessageFormatterFunction
 from buildbot.process.buildstep import EXCEPTION
 from buildbot.process.buildstep import SUCCESS
@@ -37,6 +38,7 @@ from twisted.internet import defer
 from twisted.logger import Logger
 
 from .binary_cache import S3BinaryCacheConfig
+from .message_formatter import ReasonableMessageFormatter, CallbackPayloadBuild, CallbackPayloadBuildSet, CallbackReturn
 
 log = Logger()
 
@@ -589,10 +591,10 @@ def nix_eval_config(
     project: GerritProject,
     worker_names: list[str],
     supported_systems: list[str],
-    eval_lock: util.MasterLock,
+    eval_lock: MasterLock,
     worker_count: int,
     max_memory_size: int,
-) -> util.BuilderConfig:
+) -> BuilderConfig:
     """
     Uses nix-eval-jobs to evaluate the entrypoint of this project.
     For each evaluated attribute a new build pipeline is started.
@@ -676,7 +678,7 @@ def nix_build_config(
     build_stores: list[str],
     signing_keyfile: str | None = None,
     binary_cache_config: S3BinaryCacheConfig | None = None
-) -> util.BuilderConfig:
+) -> BuilderConfig:
     """Builds one nix flake attribute."""
     factory = util.BuildFactory()
 
@@ -806,11 +808,11 @@ def config_for_project(
     nix_supported_systems: list[str],
     nix_eval_worker_count: int,
     nix_eval_max_memory_size: int,
-    eval_lock: util.MasterLock,
+    eval_lock: MasterLock,
     nix_builders: list[NixBuilder],
     signing_keyfile: str | None = None,
     binary_cache_config: S3BinaryCacheConfig | None = None
-) -> Project:
+) -> None:
     config["projects"].append(Project(project.name))
     config["schedulers"].extend(
         [
@@ -885,22 +887,22 @@ class PeriodicWithStartup(schedulers.Periodic):
             yield self.setState("last_build", None)
         yield super().activate()
 
-def gerritReviewFmt(url, data):
+def gerritReviewFmt(url: str, payload: CallbackPayloadBuild | CallbackPayloadBuildSet) -> CallbackReturn:
-    if 'build' not in data:
+    assert isinstance(payload, CallbackPayloadBuild), "BuildSet are not handled yet!"
-        raise ValueError('`build` is supposed to be present to format a build')
 
-    build = data['build']
+    build = payload.build
     if 'builder' not in build and 'name' not in build['builder']:
         raise ValueError('either `builder` or `builder.name` is not present in the build dictionary, unexpected format request')
 
     builderName = build['builder']['name']
 
+    log.info("Formatting a message for Gerrit, build info: '{data}'", data=json.dumps(build))
     result = build['results']
     if result == util.RETRY:
-        return dict()
+        return CallbackReturn()
 
     if builderName != f'{build["properties"].get("event.project")}/nix-eval':
-        return dict()
+        return CallbackReturn()
 
     failed = build['properties'].get('failed_builds', [[]])[0]
 
@@ -924,7 +926,7 @@ def gerritReviewFmt(url, data):
         message += "\nFor more details visit:\n"
         message += build['url'] + "\n"
 
-    return dict(message=message, labels=labels)
+    return CallbackReturn(body=message, extra_info={'labels': labels})
 
 class GerritNixConfigurator(ConfiguratorBase):
     """Janitor is a configurator which create a Janitor Builder with all needed Janitor steps"""
@@ -982,32 +984,32 @@ class GerritNixConfigurator(ConfiguratorBase):
 
         self.auth_method = auth_method
 
-    def configure(self, config: dict[str, Any]) -> None:
+    def configure(self, config_dict: dict[str, Any]) -> None:
-        worker_config = json.loads(read_secret_file(self.nix_workers_secret_name))
+        worker_config_dict = json.loads(read_secret_file(self.nix_workers_secret_name))
         worker_names = []
 
         if self.manhole is not None:
-            config["manhole"] = self.manhole
+            config_dict["manhole"] = self.manhole
 
-        config.setdefault("projects", [])
+        config_dict.setdefault("projects", [])
-        config.setdefault("secretsProviders", [])
+        config_dict.setdefault("secretsProviders", [])
-        config.setdefault("www", {
+        config_dict.setdefault("www", {
             'allowed_origins': self.allowed_origins
         })
 
-        for item in worker_config:
+        for item in worker_config_dict:
             cores = item.get("cores", 0)
             for i in range(cores):
                 for arch in self.nix_supported_systems + ["other"]:
                     worker_name = f"{item['name']}-{i:03}"
-                    config["workers"].append(worker.Worker(f"{worker_name}-{arch}", item["pass"]))
+                    config_dict["workers"].append(worker.Worker(f"{worker_name}-{arch}", item["pass"]))
                     worker_names.append(worker_name)
 
         eval_lock = util.MasterLock("nix-eval")
 
         for project in self.projects:
             config_for_project(
-                config,
+                config_dict,
                 self.gerrit_config,
                 GerritProject(name=project, private_sshkey_path=self.gerrit_sshkey_path),
                 worker_names,
@@ -1020,15 +1022,15 @@ class GerritNixConfigurator(ConfiguratorBase):
                 binary_cache_config=self.binary_cache_config
             )
 
-        config["change_source"] = self.gerrit_change_source
+        config_dict["change_source"] = self.gerrit_change_source
-        config["services"].append(
+        config_dict["services"].append(
             reporters.GerritStatusPush(self.gerrit_server, self.gerrit_user,
                                        port=self.gerrit_port,
                                        identity_file=self.gerrit_sshkey_path,
                                        generators=[
                                            # gerritReviewCB / self.url
-                                           BuildStatusGenerator(
+                                           GerritBuildEndStatusGenerator(
-                                               message_formatter=MessageFormatterFunction(
+                                               message_formatter=ReasonableMessageFormatter(
                                                    lambda data: gerritReviewFmt(self.url, data),
                                                    "plain",
                                                    want_properties=True,
@@ -1040,7 +1042,7 @@ class GerritNixConfigurator(ConfiguratorBase):
         )
 
         if self.prometheus_config is not None:
-            config['services'].append(reporters.Prometheus(port=self.prometheus_config.get('port', 9100), interface=self.prometheus_config.get('address', '')))
+            config_dict['services'].append(reporters.Prometheus(port=self.prometheus_config.get('port', 9100), interface=self.prometheus_config.get('address', '')))
 
         # Upstream defaults pretend they already do something similar
         # but they didn't work, hence the custom function.
@@ -1050,7 +1052,7 @@ class GerritNixConfigurator(ConfiguratorBase):
                 return ref
             return ref.rsplit('/', 1)[0]
 
-        config["services"].append(
+        config_dict["services"].append(
             util.OldBuildCanceller(
                 "build_canceller",
                 filters=[
@@ -1073,12 +1075,12 @@ class GerritNixConfigurator(ConfiguratorBase):
         systemd_secrets = secrets.SecretInAFile(
             dirname=os.environ["CREDENTIALS_DIRECTORY"],
         )
-        config["secretsProviders"].append(systemd_secrets)
+        config_dict["secretsProviders"].append(systemd_secrets)
 
-        config["www"].setdefault("plugins", {})
+        config_dict["www"].setdefault("plugins", {})
 
-        if "authz" not in config["www"]:
+        if "authz" not in config_dict["www"]:
-            config["www"]["authz"] = util.Authz(
+            config_dict["www"]["authz"] = util.Authz(
                 allowRules=[
                     util.AnyEndpointMatcher(role="admin", defaultDeny=False),
                     util.StopBuildEndpointMatcher(role="owner"),
@@ -1092,5 +1094,5 @@ class GerritNixConfigurator(ConfiguratorBase):
                 ],
             )
 
-        if "auth" not in config["www"] and self.auth_method is not None:
+        if "auth" not in config_dict["www"] and self.auth_method is not None:
-            config["www"]["auth"] = self.auth_method
+            config_dict["www"]["auth"] = self.auth_method

buildbot_nix/message_formatter.py

@@ -0,0 +1,45 @@
+from typing import Any, Callable, Literal
+from buildbot.reporters.message import MessageFormatterBase
+import dataclasses
+
+@dataclasses.dataclass
+class CallbackPayloadBuild:
+    # buddy i have no idea what the fuck is in this
+    build: dict[str, Any]
+
+@dataclasses.dataclass
+class CallbackPayloadBuildSet:
+    buildset: dict[str, Any]
+    # i have no idea what the fuck is in this honestly
+    builds: Any
+
+@dataclasses.dataclass
+class CallbackReturn:
+    body: str | None = None
+    subject: str | None = None
+    extra_info: dict[str, Any] | None = None
+    # FIXME: support other template types, if they actually become necessary
+    template_type: Literal['plain'] = 'plain'
+
+class ReasonableMessageFormatter(MessageFormatterBase):
+    """
+    Message formatter which uses strongly typed data classes to reduce suffering slightly.
+    """
+    CallbackFunc = Callable[[CallbackPayloadBuild | CallbackPayloadBuildSet], CallbackReturn]
+    def __init__(self, function: CallbackFunc, template_type: str, **kwargs):
+        super().__init__(**kwargs)
+        self.template_type = template_type
+        self._function = function
+
+    def format_message_for_build(self, master, build, **kwargs):
+        return dataclasses.asdict(self._function(CallbackPayloadBuild(build=build)))
+
+    def format_message_for_buildset(self, master, buildset, builds, **kwargs):
+        return dataclasses.asdict(self._function(CallbackPayloadBuildSet(buildset=buildset, builds=builds)))
+
+    # These only exist as callbacks, the only one actually used is render_message_dict
+    def render_message_body(self, context):
+        return None
+
+    def render_message_subject(self, context):
+        return None