address review comments

redhat-developer · Jan 7, 2025 · 74cf4c1 · 74cf4c1
1 parent ab7d3dc
commit 74cf4c1
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 91 deletions.
diff --git a/workspaces/marketplace/plugins/catalog-backend-module-marketplace/src/module.ts b/workspaces/marketplace/plugins/catalog-backend-module-marketplace/src/module.ts
@@ -39,13 +39,13 @@ export const catalogModuleMarketplace = createBackendModule({
         auth: coreServices.auth,
       },
       async init({ logger, catalog, discovery, auth }) {
-        logger.info('Marketplace module initialized!');
+        logger.info('Adding Marketplace processors to catalog...');
         catalog.addProcessor(new MarketplacePluginProcessor());
         catalog.addProcessor(new MarketplacePluginListProcessor());
+        catalog.addProcessor(new StaticPluginInstallStatusProcessor());
         catalog.addProcessor(
           new DynamicPluginInstallStatusProcessor(discovery, auth),
         );
-        catalog.addProcessor(new StaticPluginInstallStatusProcessor());
       },
     });
   },

diff --git a/.../catalog-backend-module-marketplace/src/processors/DynamicPluginInstallStatusProcessor.ts b/.../catalog-backend-module-marketplace/src/processors/DynamicPluginInstallStatusProcessor.ts
@@ -16,9 +16,11 @@
 
 import { AuthService, DiscoveryService } from '@backstage/backend-plugin-api';
 import { Entity, stringifyEntityRef } from '@backstage/catalog-model';
+import { LocationSpec } from '@backstage/plugin-catalog-common';
 import {
   CatalogProcessor,
   CatalogProcessorCache,
+  CatalogProcessorEmit,
 } from '@backstage/plugin-catalog-node';
 import { durationToMilliseconds } from '@backstage/types';
 import {
@@ -60,6 +62,7 @@ export class DynamicPluginInstallStatusProcessor implements CatalogProcessor {
       onBehalfOf: await this.auth.getOwnServiceCredentials(),
       targetPluginId: 'catalog',
     });
+
     const response = await fetch(`${scalprumUrl}/plugins`, {
       headers: {
         'Content-Type': 'application/json',
@@ -80,7 +83,7 @@ export class DynamicPluginInstallStatusProcessor implements CatalogProcessor {
   async getCachedPlugins(
     cache: CatalogProcessorCache,
     entityRef: string,
-  ): Promise<any> {
+  ): Promise<CachedData> {
     let cachedData = (await cache.get(entityRef)) as CachedData;
     if (!cachedData || this.isExpired(cachedData)) {
       const plugins = await this.getInstalledPlugins();
@@ -104,20 +107,23 @@ export class DynamicPluginInstallStatusProcessor implements CatalogProcessor {
 
   async preProcessEntity(
     entity: Entity,
-    _: any,
-    __: any,
-    ___: any,
+    _location: LocationSpec,
+    _emit: CatalogProcessorEmit,
+    _originLocation: LocationSpec,
     cache: CatalogProcessorCache,
   ): Promise<MarketplacePluginEntry> {
     if (
       entity.apiVersion === MARKETPLACE_API_VERSION &&
       entity.kind === MarketplaceKinds.plugin
     ) {
+      if (entity.spec?.installStatus === InstallStatus.Installed) {
+        return entity;
+      }
+
       const entityRef = stringifyEntityRef(entity);
 
       const data = await this.getCachedPlugins(cache, entityRef);
       const installedPluginNames = Object.keys(data?.plugins);
-
       return {
         ...entity,
         spec: {

diff --git a/...s/catalog-backend-module-marketplace/src/processors/StaticPluginInstallStatusProcessor.ts b/...s/catalog-backend-module-marketplace/src/processors/StaticPluginInstallStatusProcessor.ts
@@ -1,5 +1,5 @@
 /*
- * Copyright 2025 The Backstage Authors
+ * Copyright Red Hat, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -13,10 +13,10 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-const fs = require('fs');
-const path = require('path');
-const semver = require('semver');
 
+import fs from 'fs';
+import path from 'path';
+import semver from 'semver';
 import { Entity } from '@backstage/catalog-model';
 import { CatalogProcessor } from '@backstage/plugin-catalog-node';
 import {
@@ -67,7 +67,7 @@ export class StaticPluginInstallStatusProcessor implements CatalogProcessor {
 
       currentPath = path.dirname(currentPath);
     }
-    return false;
+    return '';
   }
 
   private isPackageInstalled(
@@ -132,48 +132,43 @@ export class StaticPluginInstallStatusProcessor implements CatalogProcessor {
 
     try {
       const parsed = JSON.parse(str);
-      if (typeof parsed === 'object' && parsed !== null) {
-        return true;
-      }
-      return true;
+      return typeof parsed === 'object' && parsed !== null;
     } catch (e) {
       return false;
     }
   }
 
-  async preProcessEntity(
-    entity: MarketplacePluginEntry,
-    _: any,
-    __: any,
-    ___: any,
-  ): Promise<Entity> {
+  async preProcessEntity(entity: MarketplacePluginEntry): Promise<Entity> {
     if (
       entity.apiVersion === MARKETPLACE_API_VERSION &&
       entity.kind === MarketplaceKinds.plugin
     ) {
-      let installStatus: InstallStatus;
-
-      if (
-        (entity?.spec?.packages ?? []).length > 0 &&
-        this.isJSON(JSON.stringify(entity.spec?.packages?.[0])) &&
-        typeof entity.spec?.packages?.[0] !== 'string'
-      ) {
-        const pkgs = entity.spec?.packages as MarketplacePackage[];
-        installStatus = pkgs?.every(npmPackage => {
-          const versions = npmPackage?.version?.split(',');
-          return versions?.every(version =>
-            this.customPaths.some(cpath =>
-              this.isPackageInstalled(npmPackage?.name, cpath, version),
-            ),
-          );
-        })
-          ? InstallStatus.Installed
-          : InstallStatus.NotInstalled;
-      } else {
-        const pkgs = entity.spec?.packages as string[];
-        installStatus = pkgs?.every(pkg =>
-          this.customPaths.some(cpath => this.isPackageInstalled(pkg, cpath)),
-        )
+      let installStatus: InstallStatus = InstallStatus.NotInstalled;
+
+      if (entity?.spec?.packages?.length) {
+        const somePackagesInstalled = entity.spec.packages.some(
+          marketplacePackageOrString => {
+            const npmPackage =
+              typeof marketplacePackageOrString === 'string'
+                ? {
+                    name: marketplacePackageOrString,
+                  }
+                : marketplacePackageOrString;
+
+            const versions = npmPackage?.version?.split(',');
+            return versions
+              ? versions?.every(version =>
+                  this.customPaths.some(cpath =>
+                    this.isPackageInstalled(npmPackage?.name, cpath, version),
+                  ),
+                )
+              : this.customPaths.some(cpath =>
+                  this.isPackageInstalled(npmPackage?.name, cpath),
+                );
+          },
+        );
+
+        installStatus = somePackagesInstalled
           ? InstallStatus.Installed
           : InstallStatus.NotInstalled;
       }

diff --git a/...d-module-marketplace/src/processors/__tests__/DynamicPluginInstallStatusProcessor.test.ts b/...d-module-marketplace/src/processors/__tests__/DynamicPluginInstallStatusProcessor.test.ts
@@ -64,6 +64,10 @@ const discoveryService = mockServices.discovery.mock({
   },
 });
 const authService = mockServices.auth.mock();
+const locationSpec = {
+  type: '',
+  target: '',
+};
 
 describe('DynamicPluginInstallStatusProcessor', () => {
   beforeEach(() => {
@@ -166,7 +170,30 @@ describe('DynamicPluginInstallStatusProcessor', () => {
       jest.resetAllMocks();
     });
 
-    it('should return entity', async () => {
+    it('should not process if the installStatus is already set', async () => {
+      const processor = new DynamicPluginInstallStatusProcessor(
+        discoveryService,
+        authService,
+      );
+
+      const entity = await processor.preProcessEntity(
+        {
+          ...pluginEntity,
+          spec: {
+            ...pluginEntity.spec,
+            installStatus: InstallStatus.Installed,
+          },
+        },
+        locationSpec,
+        jest.fn(),
+        locationSpec,
+        cache,
+      );
+
+      expect(entity.spec?.installStatus).toBe(InstallStatus.Installed);
+    });
+
+    it('should return Installed', async () => {
       const pluginsMock = { testplugin: {}, plugin2: {} };
 
       (fetch as jest.Mock).mockResolvedValue(
@@ -180,9 +207,9 @@ describe('DynamicPluginInstallStatusProcessor', () => {
 
       const entity = await processor.preProcessEntity(
         pluginEntity,
-        null,
-        null,
-        null,
+        locationSpec,
+        jest.fn(),
+        locationSpec,
         cache,
       );
 
@@ -208,9 +235,9 @@ describe('DynamicPluginInstallStatusProcessor', () => {
 
       const result = await processor.preProcessEntity(
         entity,
-        null,
-        null,
-        null,
+        locationSpec,
+        jest.fn(),
+        locationSpec,
         cache,
       );
       expect(result.spec?.installStatus).toBe('NotInstalled');
@@ -231,9 +258,9 @@ describe('DynamicPluginInstallStatusProcessor', () => {
 
       const result = await processor.preProcessEntity(
         entity,
-        null,
-        null,
-        null,
+        locationSpec,
+        jest.fn(),
+        locationSpec,
         cache,
       );
       expect(result).toEqual(entity);

diff --git a/...nd-module-marketplace/src/processors/__tests__/StaticPluginInstallStatusProcessor.test.ts b/...nd-module-marketplace/src/processors/__tests__/StaticPluginInstallStatusProcessor.test.ts
@@ -1,5 +1,5 @@
 /*
- * Copyright 2025 The Backstage Authors
+ * Copyright Red Hat, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import {
   InstallStatus,
   MarketplacePluginEntry,
@@ -53,43 +54,28 @@ describe('StaticPluginInstallStatusProcessor', () => {
   it('should return the workspace path', async () => {
     const processor = new StaticPluginInstallStatusProcessor();
 
-    const result = await processor.preProcessEntity(
-      pluginEntity,
-      undefined,
-      undefined,
-      undefined,
-    );
+    const result = await processor.preProcessEntity(pluginEntity);
     expect(result?.spec?.installStatus).toBe(InstallStatus.NotInstalled);
   });
 
   it('should return not installed status', async () => {
     const processor = new StaticPluginInstallStatusProcessor();
 
-    const result = await processor.preProcessEntity(
-      pluginEntity,
-      undefined,
-      undefined,
-      undefined,
-    );
+    const result = await processor.preProcessEntity(pluginEntity);
     expect(result?.spec?.installStatus).toBe(InstallStatus.NotInstalled);
   });
 
-  it('should return false if the root folder does not have workspaces', async () => {
+  it('should return empty string if the root folder does not have workspaces', async () => {
     const processor = new StaticPluginInstallStatusProcessor();
 
     const result = await processor.findWorkspacesPath('../../../../../../../');
-    expect(result).toBe(false);
+    expect(result).toBe('');
   });
 
   it('should return notInstalled status if the entity does not have package version information', async () => {
     const processor = new StaticPluginInstallStatusProcessor();
 
-    const result = await processor.preProcessEntity(
-      pluginEntity,
-      undefined,
-      undefined,
-      undefined,
-    );
+    const result = await processor.preProcessEntity(pluginEntity);
     expect(result?.spec?.installStatus).toBe(InstallStatus.NotInstalled);
   });
 
@@ -108,12 +94,7 @@ describe('StaticPluginInstallStatusProcessor', () => {
         ],
       },
     };
-    const result = await processor.preProcessEntity(
-      searchBackendPlugin,
-      undefined,
-      undefined,
-      undefined,
-    );
+    const result = await processor.preProcessEntity(searchBackendPlugin);
     expect(result?.spec?.installStatus).toBe(InstallStatus.NotInstalled);
   });
 
@@ -132,12 +113,7 @@ describe('StaticPluginInstallStatusProcessor', () => {
         ],
       },
     };
-    const result = await processor.preProcessEntity(
-      searchBackendPlugin,
-      undefined,
-      undefined,
-      undefined,
-    );
+    const result = await processor.preProcessEntity(searchBackendPlugin);
     expect(result?.spec?.installStatus).toBe(InstallStatus.Installed);
   });
 
@@ -158,7 +134,7 @@ describe('StaticPluginInstallStatusProcessor', () => {
         ],
       },
     };
-    const result = await processor.preProcessEntity(searchPlugin, {}, {}, {});
+    const result = await processor.preProcessEntity(searchPlugin);
     expect(result?.spec?.installStatus).toBe(InstallStatus.NotInstalled);
   });
 
@@ -175,7 +151,7 @@ describe('StaticPluginInstallStatusProcessor', () => {
         ],
       },
     };
-    const result = await processor.preProcessEntity(searchPlugin, {}, {}, {});
+    const result = await processor.preProcessEntity(searchPlugin);
     expect(result?.spec?.installStatus).toBe(InstallStatus.Installed);
   });
 
@@ -186,7 +162,7 @@ describe('StaticPluginInstallStatusProcessor', () => {
       ...pluginEntity,
       kind: 'TestKind',
     };
-    const result = await processor.preProcessEntity(testEntity, {}, {}, {});
+    const result = await processor.preProcessEntity(testEntity);
     expect(result).toEqual(testEntity);
   });