ChangesCheckerConsole: Start exactly matching job or all partially ma…

…tching jobs (#2144)

Previously, the first job with a partially matching content scope was
started.
Doing so could lead to problems when multiple jobs with overlapping
content scopes exist.
For instance, jobs with the scopes `{ domain: "main", language: "de" }`
and `{ domain: "main", language: "en" }` both partially match a change
in `{ domain: "main", language: "de" }`.
To fix this, we either start a single job if the content scope matches
exactly or start all jobs with partially matching content scopes.

.changeset/thin-balloons-invite.md

@@ -0,0 +1,10 @@
+---
+"@comet/cms-api": patch
+---
+
+ChangesCheckerConsole: Start exactly matching job or all partially matching jobs
+
+Previously, the first job with a partially matching content scope was started.
+Doing so could lead to problems when multiple jobs with overlapping content scopes exist.
+For instance, jobs with the scopes `{ domain: "main", language: "de" }` and `{ domain: "main", language: "en" }` both partially match a change in `{ domain: "main", language: "de" }`.
+To fix this, we either start a single job if the content scope matches exactly or start all jobs with partially matching content scopes.

packages/api/cms-api/package.json

@@ -92,6 +92,7 @@
         "@nestjs/core": "^9.0.0",
         "@nestjs/graphql": "^10.0.0",
         "@nestjs/platform-express": "^9.0.0",
+        "@nestjs/testing": "^9.0.0",
         "@types/express": "^4.0.0",
         "@types/jest": "^29.5.0",
         "@types/jsonwebtoken": "^8.5.9",

packages/api/cms-api/src/builds/builds.service.spec.ts

@@ -0,0 +1,93 @@
+import { V1CronJob } from "@kubernetes/client-node";
+import { getRepositoryToken } from "@mikro-orm/nestjs";
+import { Test, TestingModule } from "@nestjs/testing";
+
+import { KubernetesModule } from "../kubernetes/kubernetes.module";
+import { ACCESS_CONTROL_SERVICE } from "../user-permissions/user-permissions.constants";
+import { BuildTemplatesService } from "./build-templates.service";
+import { CONTENT_SCOPE_ANNOTATION } from "./builds.constants";
+import { BuildsService } from "./builds.service";
+import { ChangesSinceLastBuild } from "./entities/changes-since-last-build.entity";
+
+const jobMain = {
+    metadata: {
+        name: "main",
+        annotations: {
+            [CONTENT_SCOPE_ANNOTATION]: '{"domain":"main"}',
+        },
+    },
+};
+
+const jobMainEnglish = {
+    metadata: {
+        name: "main-en",
+        annotations: {
+            [CONTENT_SCOPE_ANNOTATION]: '{"domain":"main","language":"en"}',
+        },
+    },
+};
+
+const jobMainGerman = {
+    metadata: {
+        name: "main-de",
+        annotations: {
+            [CONTENT_SCOPE_ANNOTATION]: '{"domain":"main","language":"de"}',
+        },
+    },
+};
+
+const mockedBuildTemplatesService = {
+    getAllBuilderCronJobs: jest.fn<Promise<V1CronJob[]>, never[]>().mockResolvedValue([jobMainEnglish, jobMainGerman]),
+};
+
+describe("BuildsService", () => {
+    let service: BuildsService;
+
+    beforeEach(async () => {
+        const module: TestingModule = await Test.createTestingModule({
+            imports: [KubernetesModule.register({ helmRelease: "test" })],
+            providers: [
+                BuildsService,
+                { provide: getRepositoryToken(ChangesSinceLastBuild), useValue: {} },
+                { provide: BuildTemplatesService, useValue: mockedBuildTemplatesService },
+                { provide: ACCESS_CONTROL_SERVICE, useValue: {} },
+            ],
+        }).compile();
+
+        service = module.get<BuildsService>(BuildsService);
+    });
+
+    describe("getBuilderCronJobsToStart", () => {
+        it("should return single job for exact match", async () => {
+            await expect(service.getBuilderCronJobsToStart([{ domain: "main", language: "en" }])).resolves.toEqual([jobMainEnglish]);
+        });
+
+        it("should return multiple jobs for multiple exact matches", async () => {
+            await expect(
+                service.getBuilderCronJobsToStart([
+                    { domain: "main", language: "en" },
+                    { domain: "main", language: "de" },
+                ]),
+            ).resolves.toEqual([jobMainEnglish, jobMainGerman]);
+        });
+
+        it("should return all partially matching jobs", async () => {
+            await expect(service.getBuilderCronJobsToStart([{ domain: "main" }])).resolves.toEqual([jobMainEnglish, jobMainGerman]);
+
+            // Multiple content scopes in a single builder cron job.
+            mockedBuildTemplatesService.getAllBuilderCronJobs.mockResolvedValueOnce([jobMain]);
+            await expect(
+                service.getBuilderCronJobsToStart([
+                    { domain: "main", language: "en" },
+                    { domain: "main", language: "de" },
+                ]),
+            ).resolves.toEqual([jobMain]);
+        });
+
+        it("should throw an error if no job is found", async () => {
+            await expect(service.getBuilderCronJobsToStart([{ domain: "tertiary" }])).rejects.toThrow(
+                'Found changes in scope {"domain":"tertiary"} but no matching builder cron job!',
+            );
+        });
+    });
+});

packages/api/cms-api/src/builds/builds.service.ts

@@ -1,7 +1,7 @@
 import { V1CronJob, V1Job } from "@kubernetes/client-node";
 import { InjectRepository } from "@mikro-orm/nestjs";
 import { EntityRepository } from "@mikro-orm/postgresql";
-import { Inject, Injectable } from "@nestjs/common";
+import { Inject, Injectable, Logger } from "@nestjs/common";
 import parser from "cron-parser";
 import { format } from "date-fns";
 
@@ -22,6 +22,8 @@ const JOB_HISTORY_LIMIT = 20;
 
 @Injectable()
 export class BuildsService {
+    private readonly logger = new Logger(BuildsService.name);
+
     constructor(
         @InjectRepository(ChangesSinceLastBuild) private readonly changesRepository: EntityRepository<ChangesSinceLastBuild>,
         private readonly buildTemplatesService: BuildTemplatesService,
@@ -150,4 +152,49 @@ export class BuildsService {
     async getScopesWithChanges(): Promise<ContentScope[]> {
         return (await this.changesRepository.find({ scope: { $ne: "all" } })).map((change) => change.scope) as ContentScope[];
     }
+
+    async getBuilderCronJobsToStart(scopesWithChanges: ContentScope[]): Promise<V1CronJob[]> {
+        const builderCronJobs = await this.buildTemplatesService.getAllBuilderCronJobs();
+
+        const getMatchingBuilderCronJobs = (scope: ContentScope) => {
+            const matchingCronJobs: V1CronJob[] = [];
+
+            for (const cronJob of builderCronJobs) {
+                const cronJobScope = this.kubernetesService.getContentScope(cronJob);
+
+                if (!cronJobScope) {
+                    this.logger.warn(`CronJob ${cronJob.metadata?.name} has no scope, skipping...`);
+                    continue;
+                }
+
+                // Exact match between job's scope and the scope with changes.
+                if (Object.entries(cronJobScope).every(([key, value]) => (scope as Record<string, unknown>)[key] === value)) {
+                    return [cronJob];
+                }
+
+                // Check if scopes match partially. For instance, a job's scope may be { "domain": "main" }, but the change was in
+                // { "domain": "main", "language": "en" }. Or the job's scope may be { "domain": "main", "language": "en" }, but the change
+                // was in { "domain": "main" }. In both cases, the job should still be started.
+                if (Object.entries(cronJobScope).some(([key, value]) => (scope as Record<string, unknown>)[key] === value)) {
+                    matchingCronJobs.push(cronJob);
+                }
+            }
+
+            if (matchingCronJobs.length === 0) {
+                throw new Error(`Found changes in scope ${JSON.stringify(scope)} but no matching builder cron job!`);
+            }
+
+            return matchingCronJobs;
+        };
+
+        const uniqueMatchingCronJobs = new Set<V1CronJob>();
+
+        for (const scope of scopesWithChanges) {
+            for (const job of getMatchingBuilderCronJobs(scope)) {
+                uniqueMatchingCronJobs.add(job);
+            }
+        }
+
+        return Array.from(uniqueMatchingCronJobs);
+    }
 }

packages/api/cms-api/src/builds/changes-checker.console.ts

@@ -1,20 +1,17 @@
 import { MikroORM, UseRequestContext } from "@mikro-orm/core";
-import { Injectable } from "@nestjs/common";
+import { Injectable, Logger } from "@nestjs/common";
 import { Command, Console } from "nestjs-console";
 
-import { KubernetesService } from "../kubernetes/kubernetes.service";
-import { ContentScope } from "../user-permissions/interfaces/content-scope.interface";
-import { BuildTemplatesService } from "./build-templates.service";
 import { BuildsService } from "./builds.service";
 
 @Injectable()
 @Console()
 export class ChangesCheckerConsole {
+    private readonly logger = new Logger(ChangesCheckerConsole.name);
+
     constructor(
         private readonly orm: MikroORM, // MikroORM is injected so we can use the request context
         private readonly buildsService: BuildsService,
-        private readonly buildTemplateService: BuildTemplatesService,
-        private readonly kubernetesService: KubernetesService,
     ) {}
 
     @Command({
@@ -23,42 +20,25 @@ export class ChangesCheckerConsole {
     })
     @UseRequestContext()
     async execute(): Promise<void> {
-        console.log("Checking if changes since last build occurred...");
+        this.logger.log("Checking if changes since last build occurred...");
 
         if (await this.buildsService.hasChangesSinceLastBuild()) {
             if (await this.buildsService.shouldRebuildAllScopes()) {
-                console.log("Starting build(s) for all scopes...");
+                this.logger.log("Starting build(s) for all scopes...");
                 await this.buildsService.createBuildsForAllScopes("changesDetected");
             } else {
-                const builderCronJobs = await this.buildTemplateService.getAllBuilderCronJobs();
-
-                const getMatchingBuilderCronJob = (scope: ContentScope) => {
-                    for (const cronJob of builderCronJobs) {
-                        const cronJobScope = this.kubernetesService.getContentScope(cronJob);
-
-                        // Check if scopes match partially. For instance, a job's scope may be { "domain": "main" }, but the change was in
-                        // { "domain": "main", "language": "en" }. Or the job's scope may be { "domain": "main", "language": "en" }, but the change
-                        // was in { "domain": "main" }. In both cases, the job should still be started.
-                        if (Object.entries(cronJobScope ?? {}).some(([key, value]) => (scope as Record<string, unknown>)[key] === value)) {
-                            return cronJob;
-                        }
-                    }
-
-                    throw new Error(`Found changes in scope ${JSON.stringify(scope)} but no matching builder cron job!`);
-                };
-
                 const scopesWithChanges = await this.buildsService.getScopesWithChanges();
-                const builderCronJobsToStart = scopesWithChanges.map((scope) => getMatchingBuilderCronJob(scope));
+                const builderCronJobsToStart = await this.buildsService.getBuilderCronJobsToStart(scopesWithChanges);
 
-                console.log(`Starting build(s) for scopes: ${JSON.stringify(scopesWithChanges)}...`);
+                this.logger.log(`Starting build(s) for scopes: ${JSON.stringify(scopesWithChanges)}...`);
                 await this.buildsService.createBuilds("changesDetected", builderCronJobsToStart);
             }
 
-            console.log("Build(s) successfully started, resetting changesSinceLastBuild...");
+            this.logger.log("Build(s) successfully started, resetting changesSinceLastBuild...");
             await this.buildsService.deleteChangesSinceLastBuild();
-            console.log("Resetting changesSinceLastBuild successful!");
+            this.logger.log("Resetting changesSinceLastBuild successful!");
         } else {
-            console.log("No changes detected, skipping build...");
+            this.logger.log("No changes detected, skipping build...");
         }
     }
 }

packages/api/cms-api/src/kubernetes/kubernetes.service.ts

@@ -217,6 +217,10 @@ export class KubernetesService {
                 json = JSON.parse(json);
             }
 
+            if (typeof json !== "object" || json === null || Object.keys(json).length === 0) {
+                return null;
+            }
+
             return json;
         }