reveiw comments

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

plugins/catalog-backend-module-github/src/events/GithubScmEventsBridge.ts

@@ -71,7 +71,7 @@ export class GithubScmEventsBridge {
 
     if (this.#shuttingDown) {
       this.#logger.warn(
-        `Skipping GitHub webhook event of type "${params.topic}" because the bridge is shutting down`,
+        `Skipping GitHub webhook event of type "${eventType}" on topic "${params.topic}" because the bridge is shutting down`,
       );
       return;
     }
@@ -93,15 +93,15 @@ export class GithubScmEventsBridge {
           await this.#catalogScmEvents.publish(output.events);
         } else if (output.result === 'ignored') {
           this.#logger.debug(
-            `Skipping GitHub webhook event of type "${params.topic}" because it is ignored: ${output.reason}`,
+            `Skipping GitHub webhook event of type "${eventType}" on topic "${params.topic}" because it is ignored: ${output.reason}`,
           );
         } else if (output.result === 'aborted') {
           this.#logger.warn(
-            `Skipping GitHub webhook event of type "${params.topic}" because it is aborted: ${output.reason}`,
+            `Skipping GitHub webhook event of type "${eventType}" on topic "${params.topic}" because it is aborted: ${output.reason}`,
           );
         } else if (output.result === 'unsupported-event') {
           this.#logger.debug(
-            `Skipping GitHub webhook event of type "${params.topic}" because it is unsupported: ${output.event}`,
+            `Skipping GitHub webhook event of type "${eventType}" on topic "${params.topic}" because it is unsupported: ${output.event}`,
           );
         }
       } catch (error) {

plugins/catalog-backend-module-github/src/events/analyzeGithubWebhookEvent.ts

@@ -334,7 +334,7 @@ async function onPushEvent(
   for (const eventCommit of interestingShorthandCommits) {
     // As noted in the getCommit documentation, if there's a large number of
     // files in the commit then only at most 300 of them will be returned along
-    // with pagination link heasder, and then going up to a total of at most
+    // with pagination link header, and then going up to a total of at most
     // 3000 files. But we also want to use the convenient octokit API so we
     // paginate in this kind of clunky way and end whenever there's no more rel
     // next URL.

plugins/catalog-backend/src/providers/DefaultLocationStore.ts

@@ -280,7 +280,7 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
     for (const batch of chunk(Array.from(urls), 100)) {
       const existingUrls = await this.db<DbLocationsRow>('locations')
         .where('type', '=', 'url')
-        .where('target', 'in', batch)
+        .whereIn('target', batch)
         .select()
         .then(rows => new Set(rows.map(row => row.target)));
 
@@ -313,14 +313,13 @@ export class DefaultLocationStore implements LocationStore, EntityProvider {
     for (const batch of chunk(Array.from(urls), 100)) {
       const rows = await this.db<DbLocationsRow>('locations')
         .where('type', '=', 'url')
-        .where('target', 'in', batch)
+        .whereIn('target', batch)
         .select();
 
       if (rows.length) {
         await this.db<DbLocationsRow>('locations')
-          .where(
+          .whereIn(
             'id',
-            'in',
             rows.map(row => row.id),
           )
           .delete();

plugins/catalog-backend/src/util/readScmEventHandlingConfig.test.ts

@@ -22,9 +22,9 @@ describe('readScmEventHandlingConfig', () => {
     const config = new ConfigReader({});
 
     expect(readScmEventHandlingConfig(config)).toEqual({
-      refresh: true,
-      unregister: true,
-      move: true,
+      refresh: false,
+      unregister: false,
+      move: false,
     });
   });
 

plugins/catalog-backend/src/util/readScmEventHandlingConfig.ts

@@ -15,7 +15,6 @@
  */
 
 import { Config } from '@backstage/config';
-import lodash from 'lodash';
 
 export interface ScmEventHandlingConfig {
   refresh: boolean;
@@ -40,17 +39,17 @@ export function readScmEventHandlingConfig(
 ): ScmEventHandlingConfig {
   const rootKey = 'catalog.scmEvents';
 
-  if (!config.has(rootKey) || config.get(rootKey) === true) {
-    return { ...defaults };
-  } else if (config.get(rootKey) === false) {
+  if (!config.has(rootKey) || config.get(rootKey) === false) {
     return { ...disabled };
+  } else if (config.get(rootKey) === true) {
+    return { ...defaults };
   }
 
-  const given = {
-    refresh: config.getOptionalBoolean(`${rootKey}.refresh`),
-    unregister: config.getOptionalBoolean(`${rootKey}.unregister`),
-    move: config.getOptionalBoolean(`${rootKey}.move`),
+  return {
+    refresh:
+      config.getOptionalBoolean(`${rootKey}.refresh`) ?? defaults.refresh,
+    unregister:
+      config.getOptionalBoolean(`${rootKey}.unregister`) ?? defaults.unregister,
+    move: config.getOptionalBoolean(`${rootKey}.move`) ?? defaults.move,
   };
-
-  return lodash.merge({}, defaults, given);
 }

plugins/catalog-node/src/scmEvents/DefaultCatalogScmEventsService.ts

@@ -26,7 +26,7 @@ import {
  * @internal
  * @remarks
  *
- * This implementation is in-memory, which requires the produceers and consumer
+ * This implementation is in-memory, which requires the producers and consumer
  * (the catalog backend) to be deployed together.
  */
 export class DefaultCatalogScmEventsService implements CatalogScmEventsService {

plugins/catalog-node/src/scmEvents/catalogScmEventsServiceRef.ts

@@ -29,7 +29,7 @@ import { DefaultCatalogScmEventsService } from './DefaultCatalogScmEventsService
  * @remarks
  *
  * The default implementation of this service acts in-memory, which requires the
- * produceers and consumer (the catalog backend) to be deployed together.
+ * producers and consumer (the catalog backend) to be deployed together.
  */
 export const catalogScmEventsServiceRef =
   createServiceRef<CatalogScmEventsService>({

plugins/catalog-node/src/scmEvents/types.ts

@@ -44,8 +44,15 @@ export interface CatalogScmEventsService {
   };
 
   /**
-   * Publish an event to all subscribers. Returns once all subscribers have
-   * acknowledged that they have received and handled the event.
+   * Publish an event to all subscribers.
+   *
+   * @remarks
+   *
+   * This call blocks until all subscribers have either acknowledged that they
+   * have received and handled the event, or thrown an error. There are no
+   * re-sends or dead letter queues; receivers must implement a suitable
+   * resilience model themselves internally if they want to have better delivery
+   * guarantees.
    */
   publish(events: CatalogScmEvent[]): Promise<void>;
 }
@@ -64,7 +71,7 @@ export type CatalogScmEventContext = {
 
 /**
  * Represents a high level change event that happened in a source control
- * management system. THese are usually produced as a distilled version of an
+ * management system. These are usually produced as a distilled version of an
  * incoming webhook event or similar.
  *
  * @alpha
@@ -102,7 +109,7 @@ export type CatalogScmEvent =
        *
        * @remarks
        *
-       * This typically means that an individual file was created in an existing
+       * This typically means that an individual file was deleted in an existing
        * repository, for example through a git push or merge.
        */
       type: 'location.deleted';
@@ -160,7 +167,7 @@ export type CatalogScmEvent =
        *
        * @remarks
        *
-       * This typically refres to a repository being renamed, or transferred to
+       * This typically refers to a repository being renamed, or transferred to
        * a different owner. It can also refer to a change of base branch, which
        * effectively changes the base URL for many repository URL patterns.
        *