From f5f7ee5af622f9795df397c66150fbb679b2d0af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Tue, 31 Dec 2024 17:41:48 +0100 Subject: [PATCH 1/2] feat: cache conclusion and stats in database --- .../migrations/20241231154644_build-status.js | 19 +++ apps/backend/db/structure.sql | 6 +- .../api/handlers/finalizeBuilds.e2e.test.ts | 4 +- .../src/api/schema/primitives/build.ts | 4 +- .../backend/src/build-notification/comment.ts | 3 +- apps/backend/src/build/concludeBuild.ts | 50 +++++++ apps/backend/src/build/createBuild.ts | 5 +- apps/backend/src/build/createBuildDiffs.ts | 2 + apps/backend/src/build/job.e2e.test.ts | 3 +- apps/backend/src/build/job.ts | 30 ++--- apps/backend/src/build/label.ts | 4 +- .../src/database/models/Build.e2e.test.ts | 12 +- apps/backend/src/database/models/Build.ts | 126 +++++++++++++----- .../graphql/__generated__/resolver-types.ts | 22 +-- .../src/graphql/__generated__/schema.gql | 24 ++-- apps/backend/src/graphql/definitions/Build.ts | 60 +++++---- .../graphql/tests/queryRepository.e2e.test.ts | 4 +- .../screenshot-diff/computeScreenshotDiff.ts | 33 +---- apps/frontend/src/containers/Build.tsx | 91 ++++++------- .../src/containers/BuildStatusDescription.tsx | 10 +- apps/frontend/src/gql/graphql.ts | 22 +-- apps/frontend/src/pages/Build/BuildPage.tsx | 4 +- .../src/pages/Build/BuildReviewButton.tsx | 10 +- .../src/pages/Build/BuildWorkspace.tsx | 11 +- 24 files changed, 330 insertions(+), 229 deletions(-) create mode 100644 apps/backend/db/migrations/20241231154644_build-status.js create mode 100644 apps/backend/src/build/concludeBuild.ts diff --git a/apps/backend/db/migrations/20241231154644_build-status.js b/apps/backend/db/migrations/20241231154644_build-status.js new file mode 100644 index 000000000..8b1460b83 --- /dev/null +++ b/apps/backend/db/migrations/20241231154644_build-status.js @@ -0,0 +1,19 @@ +/** + * @param {import('knex').Knex} knex + */ +export const up = async (knex) => { + await knex.schema.alterTable("builds", async (table) => { + table.enum("conclusion", ["no-changes", "changes-detected"]); + table.jsonb("stats"); + }); +}; + +/** + * @param {import('knex').Knex} knex + */ +export const down = async (knex) => { + await knex.schema.alterTable("builds", async (table) => { + table.dropColumn("conclusion"); + table.dropColumn("stats"); + }); +}; diff --git a/apps/backend/db/structure.sql b/apps/backend/db/structure.sql index a47b09f62..14444c14e 100644 --- a/apps/backend/db/structure.sql +++ b/apps/backend/db/structure.sql @@ -226,7 +226,10 @@ CREATE TABLE public.builds ( metadata jsonb, "baseBranchResolvedFrom" text, "parentCommits" jsonb, + conclusion text, + stats jsonb, CONSTRAINT "builds_baseBranchResolvedFrom_check" CHECK (("baseBranchResolvedFrom" = ANY (ARRAY['user'::text, 'pull-request'::text, 'project'::text]))), + CONSTRAINT builds_conclusion_check CHECK ((conclusion = ANY (ARRAY['no-changes'::text, 'changes-detected'::text]))), CONSTRAINT builds_mode_check CHECK ((mode = ANY (ARRAY['ci'::text, 'monitoring'::text]))), CONSTRAINT builds_type_check CHECK ((type = ANY (ARRAY['reference'::text, 'check'::text, 'orphan'::text]))) ); @@ -2919,4 +2922,5 @@ INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('2024120 INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241201100628_remove-github-access-token.js', 1, NOW()); INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241201210158_google-account.js', 1, NOW()); INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241201222408_gitlab-last-loggedat.js', 1, NOW()); -INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241215091232_project-default-role.js', 1, NOW()); \ No newline at end of file +INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241215091232_project-default-role.js', 1, NOW()); +INSERT INTO public.knex_migrations(name, batch, migration_time) VALUES ('20241231154644_build-status.js', 1, NOW()); \ No newline at end of file diff --git a/apps/backend/src/api/handlers/finalizeBuilds.e2e.test.ts b/apps/backend/src/api/handlers/finalizeBuilds.e2e.test.ts index 57b60806e..02ec31b04 100644 --- a/apps/backend/src/api/handlers/finalizeBuilds.e2e.test.ts +++ b/apps/backend/src/api/handlers/finalizeBuilds.e2e.test.ts @@ -71,7 +71,7 @@ describe("finalizeBuilds", () => { { id: build.id, number: 1, - status: "stable", + status: "no-changes", url: "http://localhost:3000/awesome-team/awesome-project/builds/1", notification: { description: "Everything's good!", @@ -122,7 +122,7 @@ describe("finalizeBuilds", () => { { id: build.id, number: 1, - status: "stable", + status: "no-changes", url: "http://localhost:3000/awesome-team/awesome-project/builds/1", notification: { description: "Everything's good!", diff --git a/apps/backend/src/api/schema/primitives/build.ts b/apps/backend/src/api/schema/primitives/build.ts index a6f8cbf85..ff3dade73 100644 --- a/apps/backend/src/api/schema/primitives/build.ts +++ b/apps/backend/src/api/schema/primitives/build.ts @@ -46,13 +46,13 @@ function getBuildNotificationTypeFromBuildStatus( return "diff-accepted"; case "rejected": return "diff-rejected"; - case "diffDetected": + case "changes-detected": return "diff-detected"; case "pending": return "queued"; case "progress": return "progress"; - case "stable": + case "no-changes": return "no-diff-detected"; default: return null; diff --git a/apps/backend/src/build-notification/comment.ts b/apps/backend/src/build-notification/comment.ts index 08d7a061d..36ef793a5 100644 --- a/apps/backend/src/build-notification/comment.ts +++ b/apps/backend/src/build-notification/comment.ts @@ -66,7 +66,8 @@ export const getCommentBody = async (props: { invariant(status, "unknown build status"); const label = getBuildLabel(build.type, status); - const review = status === "diffDetected" ? ` ([Review](${url}))` : ""; + const review = + status === "changes-detected" ? ` ([Review](${url}))` : ""; const name = getBuildName({ build, project: hasMultipleProjects ? build.project : null, diff --git a/apps/backend/src/build/concludeBuild.ts b/apps/backend/src/build/concludeBuild.ts new file mode 100644 index 000000000..62ebcf0ea --- /dev/null +++ b/apps/backend/src/build/concludeBuild.ts @@ -0,0 +1,50 @@ +import { assertNever } from "@argos/util/assertNever"; +import { invariant } from "@argos/util/invariant"; + +import { job as buildNotificationJob } from "@/build-notification/job.js"; +import { transaction } from "@/database/index.js"; +import { Build, BuildConclusion, BuildNotification } from "@/database/models"; + +/** + * Concludes the build by updating the conclusion and the stats. + * Called when all diffs are processed. + */ +export async function concludeBuild(input: { buildId: string }) { + const { buildId } = input; + const statuses = await Build.getScreenshotDiffsStatuses([buildId]); + const [[conclusion], [stats]] = await Promise.all([ + Build.getConclusions([buildId], statuses), + Build.getStats([buildId]), + ]); + invariant(stats !== undefined, "No stats found"); + invariant(conclusion !== undefined, "No conclusion found"); + // If the build is not yet concluded, we don't want to update it. + if (conclusion === null) { + return; + } + const [, buildNotification] = await transaction(async (trx) => { + return Promise.all([ + Build.query(trx).findById(buildId).patch({ + conclusion, + stats, + }), + BuildNotification.query(trx).insert({ + buildId, + type: getNotificationType(conclusion), + jobStatus: "pending", + }), + ]); + }); + await buildNotificationJob.push(buildNotification.id); +} + +function getNotificationType(conclusion: BuildConclusion) { + switch (conclusion) { + case "changes-detected": + return "diff-detected"; + case "no-changes": + return "no-diff-detected"; + default: + return assertNever(conclusion); + } +} diff --git a/apps/backend/src/build/createBuild.ts b/apps/backend/src/build/createBuild.ts index a7c210c27..ed3918041 100644 --- a/apps/backend/src/build/createBuild.ts +++ b/apps/backend/src/build/createBuild.ts @@ -168,10 +168,7 @@ export async function createBuild(params: { }, ); - await pushBuildNotification({ - buildId: build.id, - type: "queued", - }); + await pushBuildNotification({ buildId: build.id, type: "queued" }); return build; } diff --git a/apps/backend/src/build/createBuildDiffs.ts b/apps/backend/src/build/createBuildDiffs.ts index b6e133e8b..d13e4f672 100644 --- a/apps/backend/src/build/createBuildDiffs.ts +++ b/apps/backend/src/build/createBuildDiffs.ts @@ -91,6 +91,8 @@ export async function createBuildDiffs(build: Build) { "no compare screenshot bucket found for build", ); + invariant(compareScreenshotBucket.complete, "compare bucket is not complete"); + const compareScreenshots = compareScreenshotBucket.screenshots; invariant(compareScreenshots, "no compare screenshots found for build"); diff --git a/apps/backend/src/build/job.e2e.test.ts b/apps/backend/src/build/job.e2e.test.ts index 8d976bec9..3805a4fef 100644 --- a/apps/backend/src/build/job.e2e.test.ts +++ b/apps/backend/src/build/job.e2e.test.ts @@ -31,7 +31,7 @@ describe("build", () => { baseScreenshotBucketId: null, compareScreenshotBucketId: compareBucket.id, projectId: project.id, - jobStatus: "pending", + jobStatus: "progress", }); const file = await factory.File.create({ type: "screenshot", @@ -43,6 +43,7 @@ describe("build", () => { }); await factory.ScreenshotBucket.create({ projectId: project.id, + complete: true, }); }); diff --git a/apps/backend/src/build/job.ts b/apps/backend/src/build/job.ts index 46d07f057..3c27fbd5d 100644 --- a/apps/backend/src/build/job.ts +++ b/apps/backend/src/build/job.ts @@ -8,19 +8,18 @@ import { createModelJob, UnretryableError } from "@/job-core/index.js"; import { job as screenshotDiffJob } from "@/screenshot-diff/index.js"; import { updateStripeUsage } from "@/stripe/index.js"; +import { concludeBuild } from "./concludeBuild.js"; import { createBuildDiffs } from "./createBuildDiffs.js"; /** * Pushes the diffs to the screenshot-diff job queue. * If there is no diff to proceed, it pushes a notification. */ -async function pushDiffs(buildId: string, screenshotDiffs: ScreenshotDiff[]) { - if (screenshotDiffs.length === 0) { - await pushBuildNotification({ buildId, type: "no-diff-detected" }); - return; - } - - const toProcessedDiffIds = screenshotDiffs +async function pushDiffs(input: { + build: Build; + screenshotDiffs: ScreenshotDiff[]; +}) { + const toProcessedDiffIds = input.screenshotDiffs .filter(({ jobStatus }) => jobStatus !== "complete") .map(({ id }) => id); @@ -29,18 +28,7 @@ async function pushDiffs(buildId: string, screenshotDiffs: ScreenshotDiff[]) { return; } - await ScreenshotDiff.fetchGraph(screenshotDiffs, "compareScreenshot"); - const statuses = await Promise.all( - screenshotDiffs.map((screenshotDiff) => screenshotDiff.$getDiffStatus()), - ); - - const type = statuses.some( - (status) => status === "added" || status === "removed", - ) - ? ("diff-detected" as const) - : ("no-diff-detected" as const); - - await pushBuildNotification({ buildId, type }); + await concludeBuild({ buildId: input.build.id }); } /** @@ -109,8 +97,8 @@ export async function performBuild(build: Build) { .withGraphFetched("[gitlabProject, account]") .throwIfNotFound(), pushBuildNotification({ buildId: build.id, type: "progress" }), - createBuildDiffs(build).then(async (diffs) => { - pushDiffs(build.id, diffs); + createBuildDiffs(build).then(async (screenshotDiffs) => { + await pushDiffs({ build, screenshotDiffs }); }), ]); diff --git a/apps/backend/src/build/label.ts b/apps/backend/src/build/label.ts index fb4ec943a..bdf54c22d 100644 --- a/apps/backend/src/build/label.ts +++ b/apps/backend/src/build/label.ts @@ -11,7 +11,7 @@ function getBuildStatusLabel(status: BuildAggregatedStatus): string { return "👍 Changes approved"; case "aborted": return "🙅 Build aborted"; - case "diffDetected": + case "changes-detected": return "⚠️ Changes detected"; case "error": return "❌ An error happened"; @@ -23,7 +23,7 @@ function getBuildStatusLabel(status: BuildAggregatedStatus): string { return "🚜 Diffing screenshots"; case "rejected": return "👎 Changes rejected"; - case "stable": + case "no-changes": return "✅ No changes detected"; default: assertNever(status); diff --git a/apps/backend/src/database/models/Build.e2e.test.ts b/apps/backend/src/database/models/Build.e2e.test.ts index 086796847..93f363828 100644 --- a/apps/backend/src/database/models/Build.e2e.test.ts +++ b/apps/backend/src/database/models/Build.e2e.test.ts @@ -233,16 +233,16 @@ describe("models/Build", () => { expect(conclusions).toEqual([null, null, null, null]); }); - it("should return 'stable' when empty", async () => { + it("should return 'no-changes' when empty", async () => { const build = await factory.Build.create({ jobStatus: "complete", }); const statuses = await Build.getStatuses([build]); const conclusions = await Build.getConclusions([build.id], statuses); - expect(conclusions).toEqual(["stable"]); + expect(conclusions).toEqual(["no-changes"]); }); - it("should return 'stable' when no diff detected", async () => { + it("should return 'no-changes' when no diff detected", async () => { const build = await factory.Build.create(); await factory.ScreenshotDiff.createMany(2, [ { buildId: build.id }, @@ -250,10 +250,10 @@ describe("models/Build", () => { ]); const statuses = await Build.getStatuses([build]); const conclusions = await Build.getConclusions([build.id], statuses); - expect(conclusions).toEqual(["stable"]); + expect(conclusions).toEqual(["no-changes"]); }); - it("should return 'diff-detected' when diff are detected", async () => { + it("should return 'changes-detected' when diff are detected", async () => { const build = await factory.Build.create(); await factory.ScreenshotDiff.createMany(2, [ { buildId: build.id }, @@ -261,7 +261,7 @@ describe("models/Build", () => { ]); const statuses = await Build.getStatuses([build]); const conclusions = await Build.getConclusions([build.id], statuses); - expect(conclusions).toEqual(["diffDetected"]); + expect(conclusions).toEqual(["changes-detected"]); }); }); diff --git a/apps/backend/src/database/models/Build.ts b/apps/backend/src/database/models/Build.ts index 4d407a9ed..6a12f48f6 100644 --- a/apps/backend/src/database/models/Build.ts +++ b/apps/backend/src/database/models/Build.ts @@ -42,11 +42,11 @@ const BuildStatusSchema = z.enum([ ]); type BuildStatus = z.infer; -const BuildConclusionSchema = z.enum(["stable", "diffDetected"]); -type BuildConclusion = z.infer | null; +const BuildConclusionSchema = z.enum(["no-changes", "changes-detected"]); +export type BuildConclusion = z.infer; const BuildReviewStatusSchema = z.enum(["accepted", "rejected"]); -type BuildReviewStatus = z.infer | null; +type BuildReviewStatus = z.infer; export const BuildAggregatedStatusSchema = z.union([ BuildReviewStatusSchema, @@ -121,6 +121,38 @@ export class Build extends Model { metadata: { oneOf: [BuildMetadataJsonSchema, { type: "null" }], }, + conclusion: { + oneOf: [ + { type: "null" }, + { type: "string", enum: ["no-changes", "changes-detected"] }, + ], + }, + stats: { + oneOf: [ + { type: "null" }, + { + type: "object", + properties: { + failure: { type: "integer" }, + added: { type: "integer" }, + unchanged: { type: "integer" }, + changed: { type: "integer" }, + removed: { type: "integer" }, + total: { type: "integer" }, + retryFailure: { type: "integer" }, + }, + required: [ + "failure", + "added", + "unchanged", + "changed", + "removed", + "total", + "retryFailure", + ], + }, + ], + }, }, }); @@ -148,6 +180,8 @@ export class Build extends Model { runAttempt!: number | null; partial!: boolean | null; metadata!: BuildMetadata | null; + conclusion!: BuildConclusion | null; + stats!: BuildStats | null; static override get relationMappings(): RelationMappings { return { @@ -245,6 +279,37 @@ export class Build extends Model { return this.reload(queryContext); } + /** + * Get screenshot diffs statuses for each build. + */ + static async getScreenshotDiffsStatuses(buildIds: string[]) { + const screenshotDiffs = buildIds.length + ? await ScreenshotDiff.query() + .select("buildId", "jobStatus") + .whereIn("buildId", buildIds) + .groupBy("buildId", "jobStatus") + : []; + + return buildIds.map((buildId) => { + const diffJobStatuses = screenshotDiffs + .filter((screenshotDiff) => screenshotDiff.buildId === buildId) + .map(({ jobStatus }) => jobStatus); + + if (diffJobStatuses.includes("error")) { + return "error"; + } + + if ( + diffJobStatuses.length === 0 || + (diffJobStatuses.length === 1 && diffJobStatuses[0] === "complete") + ) { + return "complete"; + } + + return "progress"; + }); + } + /** * Get status of the build. * Aggregate statuses from screenshot diffs. @@ -254,12 +319,8 @@ export class Build extends Model { .filter(({ jobStatus }) => jobStatus === "complete") .map(({ id }) => id); - const screenshotDiffs = completeBuildIds.length - ? await ScreenshotDiff.query() - .select("buildId", "jobStatus") - .whereIn("buildId", completeBuildIds) - .groupBy("buildId", "jobStatus") - : []; + const screenshotDiffStatuses = + await Build.getScreenshotDiffsStatuses(completeBuildIds); return builds.map((build) => { switch (build.jobStatus) { @@ -275,22 +336,10 @@ export class Build extends Model { return build.jobStatus; case "complete": { - const diffJobStatuses = screenshotDiffs - .filter(({ buildId }) => build.id === buildId) - .map(({ jobStatus }) => jobStatus); - - if (diffJobStatuses.includes("error")) { - return "error"; - } - - if ( - diffJobStatuses.length === 0 || - (diffJobStatuses.length === 1 && diffJobStatuses[0] === "complete") - ) { - return "complete"; - } - - return "progress"; + const index = completeBuildIds.indexOf(build.id); + const diffStatus = screenshotDiffStatuses[index]; + invariant(diffStatus, "diff status not found"); + return diffStatus; } default: assertNever(build.jobStatus); @@ -352,7 +401,7 @@ export class Build extends Model { static async getConclusions( buildIds: string[], statuses: BuildStatus[], - ): Promise { + ): Promise<(BuildConclusion | null)[]> { const completeBuildIds = buildIds.filter( (_, index) => statuses[index] === "complete", ); @@ -379,9 +428,9 @@ export class Build extends Model { (diff) => diff.buildId === buildId, ); if (buildDiffCount && buildDiffCount.count > 0) { - return "diffDetected"; + return "changes-detected"; } - return "stable"; + return "no-changes"; }); } @@ -390,10 +439,10 @@ export class Build extends Model { */ static async getReviewStatuses( buildIds: string[], - conclusions: BuildConclusion[], - ): Promise { + conclusions: (BuildConclusion | null)[], + ): Promise<(BuildReviewStatus | null)[]> { const diffDetectedBuildIds = buildIds.filter( - (_buildId, index) => conclusions[index] === "diffDetected", + (_buildId, index) => conclusions[index] === "changes-detected", ); const screenshotDiffs = diffDetectedBuildIds.length @@ -404,7 +453,7 @@ export class Build extends Model { : []; return buildIds.map((buildId, index) => { - if (conclusions[index] !== "diffDetected") { + if (conclusions[index] !== "changes-detected") { return null; } @@ -449,14 +498,17 @@ export class Build extends Model { return `${config.get("server.url")}${pathname}`; } - static async getAggregatedBuildStatuses(builds: Build[]) { + static async getAggregatedBuildStatuses( + builds: Build[], + ): Promise { const buildIds = builds.map((build) => build.id); const statuses = await Build.getStatuses(builds); const conclusions = await Build.getConclusions(buildIds, statuses); const reviewStatuses = await Build.getReviewStatuses(buildIds, conclusions); - return builds.map( - (_build, index) => - reviewStatuses[index] || conclusions[index] || statuses[index], - ) as BuildAggregatedStatus[]; + return builds.map((_build, index) => { + const status = + reviewStatuses[index] || conclusions[index] || statuses[index]; + return BuildAggregatedStatusSchema.parse(status); + }); } } diff --git a/apps/backend/src/graphql/__generated__/resolver-types.ts b/apps/backend/src/graphql/__generated__/resolver-types.ts index 89439ef52..9d3dd33ff 100644 --- a/apps/backend/src/graphql/__generated__/resolver-types.ts +++ b/apps/backend/src/graphql/__generated__/resolver-types.ts @@ -257,23 +257,23 @@ export type IBuildStats = { export enum IBuildStatus { /** job status: aborted */ - Aborted = 'aborted', + Aborted = 'ABORTED', /** reviewStatus: accepted */ - Accepted = 'accepted', - /** conclusion: diffDetected */ - DiffDetected = 'diffDetected', + Accepted = 'ACCEPTED', + /** conclusion: changes-detected */ + ChangesDetected = 'CHANGES_DETECTED', /** job status: complete */ - Error = 'error', + Error = 'ERROR', /** job status: expired */ - Expired = 'expired', + Expired = 'EXPIRED', + /** conclusion: no-changes */ + NoChanges = 'NO_CHANGES', /** job status: pending */ - Pending = 'pending', + Pending = 'PENDING', /** job status: progress */ - Progress = 'progress', + Progress = 'PROGRESS', /** reviewStatus: rejected */ - Rejected = 'rejected', - /** conclusion: stable */ - Stable = 'stable' + Rejected = 'REJECTED' } export enum IBuildType { diff --git a/apps/backend/src/graphql/__generated__/schema.gql b/apps/backend/src/graphql/__generated__/schema.gql index db8980462..551ac7234 100644 --- a/apps/backend/src/graphql/__generated__/schema.gql +++ b/apps/backend/src/graphql/__generated__/schema.gql @@ -225,31 +225,31 @@ type BuildStats { enum BuildStatus { """job status: aborted""" - aborted + ABORTED """reviewStatus: accepted""" - accepted + ACCEPTED - """conclusion: diffDetected""" - diffDetected + """conclusion: changes-detected""" + CHANGES_DETECTED """job status: complete""" - error + ERROR """job status: expired""" - expired + EXPIRED + + """conclusion: no-changes""" + NO_CHANGES """job status: pending""" - pending + PENDING """job status: progress""" - progress + PROGRESS """reviewStatus: rejected""" - rejected - - """conclusion: stable""" - stable + REJECTED } enum BuildType { diff --git a/apps/backend/src/graphql/definitions/Build.ts b/apps/backend/src/graphql/definitions/Build.ts index 4d034d9db..25988d51b 100644 --- a/apps/backend/src/graphql/definitions/Build.ts +++ b/apps/backend/src/graphql/definitions/Build.ts @@ -7,7 +7,7 @@ import { Build, ScreenshotDiff } from "@/database/models/index.js"; import { IBaseBranchResolution, - type IBuildStatus, + IBuildStatus, type IResolvers, } from "../__generated__/resolver-types.js"; import type { Context } from "../context.js"; @@ -28,23 +28,23 @@ export const typeDefs = gql` enum BuildStatus { "reviewStatus: accepted" - accepted + ACCEPTED "reviewStatus: rejected" - rejected - "conclusion: stable" - stable - "conclusion: diffDetected" - diffDetected + REJECTED + "conclusion: no-changes" + NO_CHANGES + "conclusion: changes-detected" + CHANGES_DETECTED "job status: pending" - pending + PENDING "job status: progress" - progress + PROGRESS "job status: complete" - error + ERROR "job status: aborted" - aborted + ABORTED "job status: expired" - expired + EXPIRED } type BuildStats { @@ -194,9 +194,29 @@ export const resolvers: IResolvers = { ); }, status: async (build, _args, ctx) => { - return ctx.loaders.BuildAggregatedStatus.load( - build, - ) as Promise; + const status = await ctx.loaders.BuildAggregatedStatus.load(build); + switch (status) { + case "accepted": + return IBuildStatus.Accepted; + case "rejected": + return IBuildStatus.Rejected; + case "no-changes": + return IBuildStatus.NoChanges; + case "changes-detected": + return IBuildStatus.ChangesDetected; + case "pending": + return IBuildStatus.Pending; + case "progress": + return IBuildStatus.Progress; + case "aborted": + return IBuildStatus.Aborted; + case "expired": + return IBuildStatus.Expired; + case "error": + return IBuildStatus.Error; + default: + assertNever(status); + } }, stats: async (build, _args, ctx) => { return ctx.loaders.BuildStats.load(build.id); @@ -286,17 +306,11 @@ export const resolvers: IResolvers = { // That might be better suited into a $afterUpdate hook. switch (validationStatus) { case "accepted": { - await pushBuildNotification({ - buildId, - type: "diff-accepted", - }); + await pushBuildNotification({ buildId, type: "diff-accepted" }); break; } case "rejected": { - await pushBuildNotification({ - buildId, - type: "diff-rejected", - }); + await pushBuildNotification({ buildId, type: "diff-rejected" }); break; } } diff --git a/apps/backend/src/graphql/tests/queryRepository.e2e.test.ts b/apps/backend/src/graphql/tests/queryRepository.e2e.test.ts index 8f4f3e53a..d3d6c69ee 100644 --- a/apps/backend/src/graphql/tests/queryRepository.e2e.test.ts +++ b/apps/backend/src/graphql/tests/queryRepository.e2e.test.ts @@ -90,12 +90,12 @@ describe("GraphQL", () => { edges: [ { number: 2, - status: "stable", + status: "NO_CHANGES", createdAt: "2017-02-05T17:14:28.167Z", }, { number: 1, - status: "stable", + status: "NO_CHANGES", createdAt: "2017-02-04T17:14:28.167Z", }, ], diff --git a/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts b/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts index e26483df5..a75d720b5 100644 --- a/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts +++ b/apps/backend/src/screenshot-diff/computeScreenshotDiff.ts @@ -4,8 +4,8 @@ import { invariant } from "@argos/util/invariant"; import type { S3Client } from "@aws-sdk/client-s3"; import type { TransactionOrKnex } from "objection"; -import { pushBuildNotification } from "@/build-notification/index.js"; -import { raw, transaction } from "@/database/index.js"; +import { concludeBuild } from "@/build/concludeBuild.js"; +import { transaction } from "@/database/index.js"; import { File, Screenshot, ScreenshotDiff } from "@/database/models/index.js"; import { S3ImageFile } from "@/storage/index.js"; import { chunk } from "@/util/chunk.js"; @@ -127,22 +127,6 @@ async function getOrCreateDiffFile(args: { return file; } -async function areAllDiffsCompleted(buildId: string): Promise<{ - complete: boolean; - diff: boolean; -}> { - const isComplete = raw(`bool_and("jobStatus" = 'complete') as complete`); - const hasDiff = raw( - `count(*) FILTER (WHERE (${ScreenshotDiff.selectDiffStatus}) in ('added', 'changed', 'removed')) > 0 AS diff`, - ); - const result = await ScreenshotDiff.query() - .select(isComplete, hasDiff) - .leftJoinRelated("compareScreenshot") - .where("buildId", buildId) - .first(); - return result as unknown as { complete: boolean; diff: boolean }; -} - /** * Computes the screenshot difference */ @@ -160,7 +144,8 @@ export const computeScreenshotDiff = async ( "[build, baseScreenshot.file, compareScreenshot.[file, screenshotBucket]]", ); - invariant(screenshotDiff?.compareScreenshot, "no compare screenshot"); + invariant(screenshotDiff.build, "no build"); + invariant(screenshotDiff.compareScreenshot, "no compare screenshot"); const baseImage = screenshotDiff.baseScreenshot?.s3Id ? new S3ImageFile({ @@ -270,12 +255,6 @@ export const computeScreenshotDiff = async ( // Unlink images await Promise.all([baseImage?.unlink(), compareImage.unlink()]); - // Push notification if all screenshot diffs are completed - const { complete, diff } = await areAllDiffsCompleted(buildId); - if (complete) { - await pushBuildNotification({ - buildId, - type: diff ? "diff-detected" : "no-diff-detected", - }); - } + // Conclude build if it's the last diff + await concludeBuild({ buildId: screenshotDiff.build.id }); }; diff --git a/apps/frontend/src/containers/Build.tsx b/apps/frontend/src/containers/Build.tsx index 1e1c30e0c..b61b92bab 100644 --- a/apps/frontend/src/containers/Build.tsx +++ b/apps/frontend/src/containers/Build.tsx @@ -12,18 +12,9 @@ import { XCircleIcon, } from "lucide-react"; -type BuildType = "reference" | "check" | "orphan"; +import { BuildStatus } from "@/gql/graphql"; -type BuildStatus = - | "accepted" - | "rejected" - | "stable" - | "diffDetected" - | "pending" - | "progress" - | "error" - | "aborted" - | "expired"; +type BuildType = "reference" | "check" | "orphan"; export const getBuildColor = ( type: BuildType | null | undefined, @@ -34,10 +25,10 @@ export const getBuildColor = ( return "success" as const; case "orphan": { switch (status) { - case "accepted": + case BuildStatus.Accepted: return "success" as const; - case "rejected": + case BuildStatus.Rejected: return "danger" as const; default: @@ -46,23 +37,23 @@ export const getBuildColor = ( } case "check": { switch (status) { - case "accepted": - case "stable": + case BuildStatus.Accepted: + case BuildStatus.NoChanges: return "success" as const; - case "error": - case "rejected": - case "expired": + case BuildStatus.Error: + case BuildStatus.Rejected: + case BuildStatus.Expired: return "danger" as const; - case "aborted": + case BuildStatus.Aborted: return "neutral" as const; - case "progress": - case "pending": + case BuildStatus.Progress: + case BuildStatus.Pending: return "pending" as const; - case "diffDetected": + case BuildStatus.ChangesDetected: return "warning" as const; default: @@ -73,8 +64,8 @@ export const getBuildColor = ( case null: case undefined: switch (status) { - case "expired": - case "error": + case BuildStatus.Expired: + case BuildStatus.Error: return "danger" as const; default: @@ -94,10 +85,10 @@ export const getBuildIcon = ( return BadgeCheckIcon; case "orphan": { switch (status) { - case "accepted": + case BuildStatus.Accepted: return ThumbsUpIcon; - case "rejected": + case BuildStatus.Rejected: return ThumbsDownIcon; default: @@ -106,27 +97,27 @@ export const getBuildIcon = ( } case "check": { switch (status) { - case "accepted": + case BuildStatus.Accepted: return ThumbsUpIcon; - case "stable": + case BuildStatus.NoChanges: return CheckCircle2Icon; - case "error": - case "expired": - case "aborted": + case BuildStatus.Error: + case BuildStatus.Expired: + case BuildStatus.Aborted: return XCircleIcon; - case "rejected": + case BuildStatus.Rejected: return ThumbsDownIcon; - case "progress": + case BuildStatus.Progress: return RefreshCcwDotIcon; - case "pending": + case BuildStatus.Pending: return CircleDotIcon; - case "diffDetected": + case BuildStatus.ChangesDetected: return AlertTriangleIcon; default: @@ -137,8 +128,8 @@ export const getBuildIcon = ( case null: case undefined: switch (status) { - case "expired": - case "error": + case BuildStatus.Expired: + case BuildStatus.Error: return XCircleIcon; default: return DotIcon; @@ -155,9 +146,9 @@ export const getBuildLabel = ( switch (type) { case "orphan": { switch (status) { - case "rejected": + case BuildStatus.Rejected: return "Changes rejected"; - case "accepted": + case BuildStatus.Accepted: return "Changes approved"; default: return "Orphan build"; @@ -167,23 +158,23 @@ export const getBuildLabel = ( return "Auto-approved build"; case "check": { switch (status) { - case "stable": + case BuildStatus.NoChanges: return "No changes detected"; - case "diffDetected": + case BuildStatus.ChangesDetected: return "Changes detected"; - case "pending": + case BuildStatus.Pending: return "Build scheduled"; - case "progress": + case BuildStatus.Progress: return "Build in progress"; - case "error": + case BuildStatus.Error: return "Build failed"; - case "aborted": + case BuildStatus.Aborted: return "Build aborted"; - case "expired": + case BuildStatus.Expired: return "Build expired"; - case "rejected": + case BuildStatus.Rejected: return "Changes rejected"; - case "accepted": + case BuildStatus.Accepted: return "Changes approved"; default: assertNever(status); @@ -193,9 +184,9 @@ export const getBuildLabel = ( case null: case undefined: switch (status) { - case "expired": + case BuildStatus.Expired: return "Build expired"; - case "error": + case BuildStatus.Error: return "Build failed"; default: return "Build scheduled"; diff --git a/apps/frontend/src/containers/BuildStatusDescription.tsx b/apps/frontend/src/containers/BuildStatusDescription.tsx index 9701eb0bd..84e4ef775 100644 --- a/apps/frontend/src/containers/BuildStatusDescription.tsx +++ b/apps/frontend/src/containers/BuildStatusDescription.tsx @@ -26,7 +26,7 @@ export const BuildStatusDescription = (props: { }) => { const build = useFragment(BuildFragment, props.build); - if (build.status === "expired") { + if (build.status === BuildStatus.Expired) { if (build.parallel) { if (build.parallel.total === -1) { return ( @@ -54,9 +54,9 @@ export const BuildStatusDescription = (props: { switch (build.type) { case "orphan": switch (build.status) { - case "accepted": + case BuildStatus.Accepted: return <>Changes have been accepted by a user.; - case "rejected": + case BuildStatus.Rejected: return <>Changes have been rejected by a user.; default: { switch (build.mode) { @@ -111,7 +111,7 @@ export const BuildStatusDescription = (props: { case "check": { switch (build.status) { - case BuildStatus.Stable: { + case BuildStatus.NoChanges: { if (build.stats.total === 0) { return ( <> @@ -129,7 +129,7 @@ export const BuildStatusDescription = (props: { case BuildStatus.Aborted: return <>This build has been voluntarily aborted.; - case BuildStatus.DiffDetected: + case BuildStatus.ChangesDetected: return ( <> Some changes have been detected between baseline and current diff --git a/apps/frontend/src/gql/graphql.ts b/apps/frontend/src/gql/graphql.ts index 38b0df920..9000e04a7 100644 --- a/apps/frontend/src/gql/graphql.ts +++ b/apps/frontend/src/gql/graphql.ts @@ -252,23 +252,23 @@ export type BuildStats = { export enum BuildStatus { /** job status: aborted */ - Aborted = 'aborted', + Aborted = 'ABORTED', /** reviewStatus: accepted */ - Accepted = 'accepted', - /** conclusion: diffDetected */ - DiffDetected = 'diffDetected', + Accepted = 'ACCEPTED', + /** conclusion: changes-detected */ + ChangesDetected = 'CHANGES_DETECTED', /** job status: complete */ - Error = 'error', + Error = 'ERROR', /** job status: expired */ - Expired = 'expired', + Expired = 'EXPIRED', + /** conclusion: no-changes */ + NoChanges = 'NO_CHANGES', /** job status: pending */ - Pending = 'pending', + Pending = 'PENDING', /** job status: progress */ - Progress = 'progress', + Progress = 'PROGRESS', /** reviewStatus: rejected */ - Rejected = 'rejected', - /** conclusion: stable */ - Stable = 'stable' + Rejected = 'REJECTED' } export enum BuildType { diff --git a/apps/frontend/src/pages/Build/BuildPage.tsx b/apps/frontend/src/pages/Build/BuildPage.tsx index bb00af8dd..ef67efe62 100644 --- a/apps/frontend/src/pages/Build/BuildPage.tsx +++ b/apps/frontend/src/pages/Build/BuildPage.tsx @@ -3,6 +3,7 @@ import { useEffect } from "react"; import { useSafeQuery } from "@/containers/Apollo"; import { PaymentBanner } from "@/containers/PaymentBanner"; import { graphql } from "@/gql"; +import { BuildStatus } from "@/gql/graphql"; import { BuildContextProvider } from "./BuildContext"; import { BuildDiffColorStateProvider } from "./BuildDiffColorState"; @@ -58,7 +59,8 @@ export const BuildPage = ({ params }: { params: BuildParams }) => { const build = project?.build ?? null; const buildStatusProgress = Boolean( build?.status && - (build.status === "pending" || build.status === "progress"), + (build.status === BuildStatus.Pending || + build.status === BuildStatus.Progress), ); const hotkeysDialog = useBuildHotkeysDialogState(); diff --git a/apps/frontend/src/pages/Build/BuildReviewButton.tsx b/apps/frontend/src/pages/Build/BuildReviewButton.tsx index 2b534eb88..1cfadb314 100644 --- a/apps/frontend/src/pages/Build/BuildReviewButton.tsx +++ b/apps/frontend/src/pages/Build/BuildReviewButton.tsx @@ -48,8 +48,8 @@ function BaseReviewButton(props: { }, }); - const AcceptIcon = getBuildIcon("check", "accepted"); - const RejectIcon = getBuildIcon("check", "rejected"); + const AcceptIcon = getBuildIcon("check", BuildStatus.Accepted); + const RejectIcon = getBuildIcon("check", BuildStatus.Rejected); return ( @@ -74,7 +74,7 @@ function BaseReviewButton(props: { }, }); }} - isDisabled={props.build.status === "accepted"} + isDisabled={props.build.status === BuildStatus.Accepted} > @@ -90,7 +90,7 @@ function BaseReviewButton(props: { }, }); }} - isDisabled={props.build.status === "rejected"} + isDisabled={props.build.status === BuildStatus.Rejected} > @@ -126,7 +126,7 @@ export function BuildReviewButton(props: { ![ BuildStatus.Accepted, BuildStatus.Rejected, - BuildStatus.DiffDetected, + BuildStatus.ChangesDetected, ].includes(project.build.status) ) { return null; diff --git a/apps/frontend/src/pages/Build/BuildWorkspace.tsx b/apps/frontend/src/pages/Build/BuildWorkspace.tsx index 75170da8e..f5b8a9492 100644 --- a/apps/frontend/src/pages/Build/BuildWorkspace.tsx +++ b/apps/frontend/src/pages/Build/BuildWorkspace.tsx @@ -2,6 +2,7 @@ import { memo } from "react"; import { BuildStatusDescription } from "@/containers/BuildStatusDescription"; import { DocumentType, FragmentType, graphql, useFragment } from "@/gql"; +import { BuildStatus } from "@/gql/graphql"; import { Progress } from "@/ui/Progress"; import { BuildDetail } from "./BuildDetail"; @@ -89,16 +90,16 @@ export function BuildWorkspace(props: { {(() => { switch (build.status) { - case "aborted": - case "error": - case "expired": + case BuildStatus.Aborted: + case BuildStatus.Error: + case BuildStatus.Expired: return (
); - case "pending": - case "progress": + case BuildStatus.Pending: + case BuildStatus.Progress: return ; default: return ( From 2f100d2d97b8659259f83ea866d5d78c7dd9586d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Greg=20Berg=C3=A9?= Date: Tue, 31 Dec 2024 23:00:49 +0100 Subject: [PATCH 2/2] chore: add job to conclude all builds in database --- Procfile | 2 +- .../src/build/bin/queue-unconcluded-builds.ts | 41 +++++++++++++++++++ apps/backend/src/build/conclude-job.ts | 16 ++++++++ apps/backend/src/job-core/createJob.ts | 4 +- .../src/processes/proc/conclude-builds.ts | 6 +++ 5 files changed, 66 insertions(+), 3 deletions(-) create mode 100755 apps/backend/src/build/bin/queue-unconcluded-builds.ts create mode 100644 apps/backend/src/build/conclude-job.ts create mode 100644 apps/backend/src/processes/proc/conclude-builds.ts diff --git a/Procfile b/Procfile index 93b8d4a7a..c4bfc1646 100644 --- a/Procfile +++ b/Procfile @@ -2,4 +2,4 @@ release: ./scripts/release.sh web: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/web.js buildAndSynchronize: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/build-and-synchronize.js screenshotDiff: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/screenshot-diff.js -crawlAndCapture: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/crawl-and-capture.js +concludeBuilds: ./scripts/heroku-node-settings.sh apps/backend/dist/processes/proc/conclude-builds.js diff --git a/apps/backend/src/build/bin/queue-unconcluded-builds.ts b/apps/backend/src/build/bin/queue-unconcluded-builds.ts new file mode 100755 index 000000000..c5826062a --- /dev/null +++ b/apps/backend/src/build/bin/queue-unconcluded-builds.ts @@ -0,0 +1,41 @@ +#!/usr/bin/env node +import { callbackify } from "node:util"; + +import { Build } from "@/database/models/index.js"; +import logger from "@/logger/index.js"; + +import { concludeBuildsJob } from "../conclude-job"; + +const main = callbackify(async () => { + const batch = 500; + const totalCount = await Build.query() + .where("jobStatus", "complete") + .whereNull("conclusion") + .resultSize(); + + let total = 0; + for (let offset = 0; offset < totalCount; offset += batch) { + const nodes = await Build.query() + .where("jobStatus", "complete") + .whereNull("conclusion") + .limit(batch) + .offset(offset) + .orderBy("id", "desc"); + + const ids = nodes.map((node) => node.id); + const percentage = Math.round((total / totalCount) * 100); + logger.info( + `Processing ${total}/${totalCount} (${percentage}%) - Pushing ${ids.length} builds in queue`, + ); + await concludeBuildsJob.push(...ids); + total += nodes.length; + } + + logger.info(`${total} builds pushed in queue (100% complete)`); +}); + +main((err) => { + if (err) { + throw err; + } +}); diff --git a/apps/backend/src/build/conclude-job.ts b/apps/backend/src/build/conclude-job.ts new file mode 100644 index 000000000..8f8b25ae4 --- /dev/null +++ b/apps/backend/src/build/conclude-job.ts @@ -0,0 +1,16 @@ +import { createJob } from "@/job-core/index.js"; +import logger from "@/logger/index.js"; + +import { concludeBuild } from "./concludeBuild.js"; + +export const concludeBuildsJob = createJob("conclude-builds", { + complete: () => {}, + error: (value, error) => { + console.error("Error while processing build", value, error); + }, + perform: async (buildId: string) => { + logger.info(`Concluding build ${buildId}`); + await concludeBuild({ buildId }); + logger.info(`Build ${buildId} concluded`); + }, +}); diff --git a/apps/backend/src/job-core/createJob.ts b/apps/backend/src/job-core/createJob.ts index 8ad2a9d6e..8e037d44a 100644 --- a/apps/backend/src/job-core/createJob.ts +++ b/apps/backend/src/job-core/createJob.ts @@ -56,7 +56,7 @@ export const createJob = ( consumer: { perform: (value: TValue) => void | Promise; complete: (value: TValue) => void | Promise; - error: (value: TValue) => void | Promise; + error: (value: TValue, error: unknown) => void | Promise; }, { prefetch = 1 }: JobParams = {}, ): Job => { @@ -145,7 +145,7 @@ export const createJob = ( scope.setExtra("jobArgs", payload.args); logger.error(error); }); - await consumer.error(payload.args[0]); + await consumer.error(payload.args[0], error); return; } } catch (error) { diff --git a/apps/backend/src/processes/proc/conclude-builds.ts b/apps/backend/src/processes/proc/conclude-builds.ts new file mode 100644 index 000000000..fe8b154b4 --- /dev/null +++ b/apps/backend/src/processes/proc/conclude-builds.ts @@ -0,0 +1,6 @@ +import "../setup.js"; + +import { concludeBuildsJob } from "@/build/conclude-job.js"; +import { createJobWorker } from "@/job-core/index.js"; + +createJobWorker(concludeBuildsJob);