review updates

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

.changeset/shiny-seas-yell.md

@@ -2,4 +2,17 @@
 '@backstage/plugin-tech-insights-backend': minor
 ---
 
-Updates tech-insights to use backend-tasks as the Fact Retriever scheduler.
+This backend now uses the `@backstage/backend-tasks` package facilities for scheduling fact retrievers.
+
+**BREAKING**: The `buildTechInsightsContext` function now takes an additional field in its options argument: `scheduler`. This is an instance of `PluginTaskScheduler`, which can be found in your backend initialization code's `env`.
+
+```diff
+ const builder = buildTechInsightsContext({
+   logger: env.logger,
+   config: env.config,
+   database: env.database,
+   discovery: env.discovery,
++  scheduler: env.scheduler,
+   factRetrievers: [ /* ... */ ],
+ });
+```

packages/backend-tasks/src/tasks/PluginTaskSchedulerImpl.ts

@@ -41,11 +41,9 @@ export class PluginTaskSchedulerImpl implements PluginTaskScheduler {
 
     // check if task exists
     const rows = await knex<DbTasksRow>(DB_TASKS_TABLE)
-      .count({ count: '*' })
+      .select(knex.raw(1))
       .where('id', '=', id);
-
-    // validate the task exists
-    if (rows[0].count !== 1) {
+    if (rows.length !== 1) {
       throw new NotFoundError(`Task ${id} does not exist`);
     }
 
@@ -56,7 +54,7 @@ export class PluginTaskSchedulerImpl implements PluginTaskScheduler {
         next_run_start_at: knex.fn.now(),
       });
     if (updatedRows < 1) {
-      throw new ConflictError(`task ${id} is currently running`);
+      throw new ConflictError(`Task ${id} is currently running`);
     }
   }
 

plugins/tech-insights-backend/README.md

@@ -35,6 +35,7 @@ export default async function createPlugin(
     config: env.config,
     database: env.database,
     discovery: env.discovery,
+    scheduler: env.scheduler,
     factRetrievers: [], // Fact retrievers registrations you want tech insights to use
   });
 

plugins/tech-insights-backend/api-report.md

@@ -27,11 +27,9 @@ export const buildTechInsightsContext: <
 ) => Promise<TechInsightsContext<CheckType, CheckResultType>>;
 
 // @public
-export function createFactRetrieverRegistration({
-  cadence,
-  factRetriever,
-  lifecycle,
-}: FactRetrieverRegistrationOptions): FactRetrieverRegistration;
+export function createFactRetrieverRegistration(
+  options: FactRetrieverRegistrationOptions,
+): FactRetrieverRegistration;
 
 // @public
 export function createRouter<

plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.test.ts

@@ -194,7 +194,7 @@ describe('FactRetrieverEngine', () => {
       }
 
       engine = await createEngine(databaseId, insertCallback, () => {});
-      engine.schedule();
+      await engine.schedule();
       const job: FactRetrieverRegistration = engine.getJobRegistration(
         testFactRetriever.id,
       );

plugins/tech-insights-backend/src/service/fact/FactRetrieverEngine.ts

@@ -39,7 +39,7 @@ function duration(startTimestamp: [number, number]): string {
 }
 
 export class FactRetrieverEngine {
-  constructor(
+  private constructor(
     private readonly repository: TechInsightsStore,
     private readonly factRetrieverRegistry: FactRetrieverRegistry,
     private readonly factRetrieverContext: FactRetrieverContext,
@@ -49,14 +49,7 @@ export class FactRetrieverEngine {
     private readonly defaultTimeout?: Duration,
   ) {}
 
-  static async create({
-    repository,
-    factRetrieverRegistry,
-    factRetrieverContext,
-    scheduler,
-    defaultCadence,
-    defaultTimeout,
-  }: {
+  static async create(options: {
     repository: TechInsightsStore;
     factRetrieverRegistry: FactRetrieverRegistry;
     factRetrieverContext: FactRetrieverContext;
@@ -64,6 +57,15 @@ export class FactRetrieverEngine {
     defaultCadence?: string;
     defaultTimeout?: Duration;
   }) {
+    const {
+      repository,
+      factRetrieverRegistry,
+      factRetrieverContext,
+      scheduler,
+      defaultCadence,
+      defaultTimeout,
+    } = options;
+
     await Promise.all(
       factRetrieverRegistry
         .listRetrievers()
@@ -81,31 +83,35 @@ export class FactRetrieverEngine {
     );
   }
 
-  schedule() {
+  async schedule() {
     const registrations = this.factRetrieverRegistry.listRegistrations();
     const newRegs: string[] = [];
-    registrations.forEach(async registration => {
-      const { factRetriever, cadence, lifecycle, timeout } = registration;
-      const cronExpression =
-        cadence || this.defaultCadence || randomDailyCron();
-      const timeLimit =
-        timeout || this.defaultTimeout || Duration.fromObject({ minutes: 5 });
-      try {
-        await this.scheduler.scheduleTask({
-          id: factRetriever.id,
-          frequency: { cron: cronExpression },
-          fn: this.createFactRetrieverHandler(factRetriever, lifecycle),
-          timeout: timeLimit,
-        });
-      } catch (e) {
-        throw new Error(
-          `Failed to schedule fact retriever ${factRetriever.id}, ${e}`,
-        );
-      }
-      newRegs.push(factRetriever.id);
-    });
+
+    await Promise.all(
+      registrations.map(async registration => {
+        const { factRetriever, cadence, lifecycle, timeout } = registration;
+        const cronExpression =
+          cadence || this.defaultCadence || randomDailyCron();
+        const timeLimit =
+          timeout || this.defaultTimeout || Duration.fromObject({ minutes: 5 });
+        try {
+          await this.scheduler.scheduleTask({
+            id: factRetriever.id,
+            frequency: { cron: cronExpression },
+            fn: this.createFactRetrieverHandler(factRetriever, lifecycle),
+            timeout: timeLimit,
+          });
+          newRegs.push(factRetriever.id);
+        } catch (e) {
+          this.logger.warn(
+            `Failed to schedule fact retriever ${factRetriever.id}, ${e}`,
+          );
+        }
+      }),
+    );
+
     this.logger.info(
-      `Scheduled ${newRegs.length} fact retrievers to Fact Retriever Engine through Backend Tasks`,
+      `Scheduled ${newRegs.length}/${registrations.length} fact retrievers into the tech-insights engine`,
     );
   }
 
@@ -114,7 +120,7 @@ export class FactRetrieverEngine {
   }
 
   async triggerJob(ref: string): Promise<void> {
-    return this.scheduler.triggerTask(ref);
+    await this.scheduler.triggerTask(ref);
   }
 
   private createFactRetrieverHandler(

plugins/tech-insights-backend/src/service/fact/createFactRetriever.ts

@@ -61,11 +61,10 @@ export type FactRetrieverRegistrationOptions = {
  * \{ maxItems: 7 \} -- This fact retriever will leave 7 newest items in the database when it is run
  *
  */
-export function createFactRetrieverRegistration({
-  cadence,
-  factRetriever,
-  lifecycle,
-}: FactRetrieverRegistrationOptions): FactRetrieverRegistration {
+export function createFactRetrieverRegistration(
+  options: FactRetrieverRegistrationOptions,
+): FactRetrieverRegistration {
+  const { cadence, factRetriever, lifecycle } = options;
   return {
     cadence,
     factRetriever,

plugins/tech-insights-backend/src/service/techInsightsContextBuilder.ts

@@ -124,7 +124,7 @@ export const buildTechInsightsContext = async <
     },
   });
 
-  factRetrieverEngine.schedule();
+  await factRetrieverEngine.schedule();
 
   if (factCheckerFactory) {
     const factChecker = factCheckerFactory.construct(