From 1874c313fc7b71c8a757ba6b49c7370847290ae7 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 24 Jun 2026 00:43:30 +0000 Subject: [PATCH] feat(#3529): move ThresholdConfig from MetricProvider to individual Metrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor the scorecard MetricProvider interface to move threshold configuration from provider-level methods to individual Metric objects. Core interface changes: - Add `threshold: ThresholdConfig` field to the `Metric` type - Remove `getMetricType()`, `getMetric()`, `getMetricThresholds()`, `calculateMetric()`, and `getMetricIds()` from MetricProvider - Make `getMetrics()` and `calculateMetrics()` required on MetricProvider - Consolidate single/batch provider code paths into batch-only Provider migrations: - All providers (dependabot, filecheck, github, jira, openssf, sonarqube) updated to the new interface with thresholds on metric objects - ThresholdResolver API: `resolveProviderThresholds(provider)` → `resolveMetricThresholds(metric, providerId)` - ThresholdResolver API: `resolveEntityThresholds(entity, provider)` → `resolveEntityThresholds(entity, metric, providerId)` - mergeEntityAndProviderThresholds signature updated similarly - PullMetricsByProviderTask consolidated to always use calculateMetrics() - MetricProvidersRegistry uses provider.getMetrics() for all operations All tests updated. API reports regenerated. Note: DatabaseMetricValues tests could not run (missing better-sqlite3 native binding in sandbox). All other 91 test suites (976 tests) passed. Closes #3529 Co-Authored-By: Claude Opus 4.6 --- .../DependabotMetricProvider.test.ts | 40 +-- .../DependabotMetricProvider.ts | 38 ++- .../DependabotMetricProviderFactory.test.ts | 6 +- .../FilecheckMetricProvider.test.ts | 82 +----- .../FilecheckMetricProvider.ts | 28 +-- .../GithubOpenPRsProvider.test.ts | 13 +- .../metricProviders/GithubOpenPRsProvider.ts | 36 ++- .../JiraOpenIssuesProvider.test.ts | 64 ++--- .../metricProviders/JiraOpenIssuesProvider.ts | 37 ++- .../OpenSSFMetricProvider.test.ts | 31 +-- .../metricProviders/OpenSSFMetricProvider.ts | 39 ++- .../SonarQubeBasicMetricProvider.test.ts | 18 +- .../SonarQubeBasicMetricProvider.ts | 27 +- .../SonarQubeBooleanMetricProvider.test.ts | 24 +- .../SonarQubeBooleanMetricProvider.ts | 10 +- .../SonarQubeMetricProviderFactory.test.ts | 10 +- .../SonarQubeNumberMetricProvider.test.ts | 27 +- .../SonarQubeNumberMetricProvider.ts | 18 +- .../mockMetricProvidersRegistry.ts | 5 +- .../__fixtures__/mockProviders.ts | 74 +++--- .../src/permissions/permissionUtils.test.ts | 1 + .../providers/MetricProvidersRegistry.test.ts | 98 ++------ .../src/providers/MetricProvidersRegistry.ts | 87 +++---- .../tasks/PullMetricsByProviderTask.test.ts | 48 ++-- .../tasks/PullMetricsByProviderTask.ts | 238 +++++++----------- .../src/service/CatalogMetricService.test.ts | 26 +- .../src/service/CatalogMetricService.ts | 3 +- .../src/service/mappers.test.ts | 1 + .../src/service/router.test.ts | 10 +- .../scorecard-backend/src/service/router.ts | 10 +- .../src/threshold/ThresholdResolver.test.ts | 63 ++++- .../src/threshold/ThresholdResolver.ts | 25 +- .../mergeEntityAndProviderThresholds.test.ts | 211 +++++++++------- .../utils/mergeEntityAndProviderThresholds.ts | 14 +- .../plugins/scorecard-common/report.api.md | 1 + .../scorecard-common/src/types/Metric.ts | 3 +- .../plugins/scorecard-node/report.api.md | 9 +- .../scorecard-node/src/api/MetricProvider.ts | 45 +--- .../plugins/scorecard/dev/legacy.tsx | 1 + .../scorecard/plugins/scorecard/dev/mocks.ts | 1 + .../src/hooks/__tests__/useMetric.test.tsx | 1 + 41 files changed, 643 insertions(+), 880 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.test.ts index 98cebb3343..d70c5a01e0 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.test.ts @@ -69,7 +69,7 @@ describe('DependabotMetricProvider', () => { }); }); - describe('getProviderId / getMetric', () => { + describe('getProviderId / getMetrics', () => { it.each([ ['critical', 'dependabot.alerts_critical', 'Dependabot Critical Alerts'], ['high', 'dependabot.alerts_high', 'Dependabot High Alerts'], @@ -84,40 +84,21 @@ describe('DependabotMetricProvider', () => { severity, ); expect(provider.getProviderId()).toBe(expectedId); - const metric = provider.getMetric(); + const metrics = provider.getMetrics(); + expect(metrics).toHaveLength(1); + const metric = metrics[0]; expect(metric.id).toBe(expectedId); expect(metric.title).toBe(expectedTitle); expect(metric.description).toBe( DEPENDABOT_SEVERITY_METRIC[severity].description, ); expect(metric.type).toBe('number'); + expect(metric.threshold).toEqual(DEPENDABOT_THRESHOLDS); expect(metric.history).toBe(true); }, ); }); - describe('getMetricType', () => { - it('returns number', () => { - const provider = new DependabotMetricProvider( - mockConfig, - mockLogger, - 'critical', - ); - expect(provider.getMetricType()).toBe('number'); - }); - }); - - describe('getMetricThresholds', () => { - it('returns default thresholds', () => { - const provider = new DependabotMetricProvider( - mockConfig, - mockLogger, - 'critical', - ); - expect(provider.getMetricThresholds()).toEqual(DEPENDABOT_THRESHOLDS); - }); - }); - describe('getCatalogFilter', () => { it('requires project-slug and dependabot annotation value true', () => { const provider = new DependabotMetricProvider( @@ -184,7 +165,7 @@ describe('DependabotMetricProvider', () => { ); }); - describe('calculateMetric', () => { + describe('calculateMetrics', () => { it.each(['critical', 'high', 'medium', 'low'] as const)( 'calls getAlerts with target from getEntitySourceLocation and returns count', async severity => { @@ -196,9 +177,9 @@ describe('DependabotMetricProvider', () => { ); const ent = entity(); - const result = await provider.calculateMetric(ent); + const results = await provider.calculateMetrics(ent); - expect(result).toBe(2); + expect(results.get(provider.getProviderId())).toBe(2); // target comes from getEntitySourceLocation(entity), not hardcoded expect(mockGetAlerts).toHaveBeenCalledWith( 'https://github.com/owner/repo', @@ -219,7 +200,8 @@ describe('DependabotMetricProvider', () => { mockLogger, 'critical', ); - expect(await provider.calculateMetric(entity())).toBe(0); + const results = await provider.calculateMetrics(entity()); + expect(results.get(provider.getProviderId())).toBe(0); }); it('propagates errors when getAlerts fails', async () => { @@ -230,7 +212,7 @@ describe('DependabotMetricProvider', () => { 'critical', ); - await expect(provider.calculateMetric(entity())).rejects.toThrow( + await expect(provider.calculateMetrics(entity())).rejects.toThrow( 'dependabot unavailable', ); expect(mockGetAlerts).toHaveBeenCalledWith( diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.ts index a2b2c3d4ae..6d2c9a68a3 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.ts @@ -14,10 +14,7 @@ * limitations under the License. */ -import { - Metric, - ThresholdConfig, -} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecard-node'; import type { LoggerService } from '@backstage/backend-plugin-api'; import type { Config } from '@backstage/config'; @@ -63,23 +60,18 @@ export class DependabotMetricProvider implements MetricProvider<'number'> { return DEPENDABOT_SEVERITY_METRIC[this.severity].id; } - getMetricType(): 'number' { - return 'number'; - } - - getMetric(): Metric<'number'> { + getMetrics(): Metric<'number'>[] { const meta = DEPENDABOT_SEVERITY_METRIC[this.severity]; - return { - id: meta.id, - title: meta.title, - description: meta.description, - type: this.getMetricType(), - history: true, - }; - } - - getMetricThresholds(): ThresholdConfig { - return DEPENDABOT_THRESHOLDS; + return [ + { + id: meta.id, + title: meta.title, + description: meta.description, + type: 'number', + threshold: DEPENDABOT_THRESHOLDS, + history: true, + }, + ]; } getCatalogFilter(): Record { @@ -112,7 +104,7 @@ export class DependabotMetricProvider implements MetricProvider<'number'> { return { owner, repo }; } - async calculateMetric(entity: Entity): Promise { + async calculateMetrics(entity: Entity): Promise> { const repository = this.getRepository(entity); const { target } = getEntitySourceLocation(entity); const alerts = await this.dependabotClient.getAlerts( @@ -120,6 +112,8 @@ export class DependabotMetricProvider implements MetricProvider<'number'> { repository, this.severity, ); - return alerts.length; + const results = new Map(); + results.set(this.getProviderId(), alerts.length); + return results; } } diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProviderFactory.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProviderFactory.test.ts index 40e3644b82..0af03c9b44 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProviderFactory.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProviderFactory.test.ts @@ -36,8 +36,10 @@ describe('createDependabotMetricProvider', () => { ); expect(provider.getProviderId()).toBe('dependabot.alerts_high'); expect(provider.getProviderDatasourceId()).toBe('dependabot'); - expect(provider.getMetricType()).toBe('number'); - expect(provider.getMetricThresholds()).toBe(DEPENDABOT_THRESHOLDS); + const metrics = provider.getMetrics(); + expect(metrics).toHaveLength(1); + expect(metrics[0].type).toBe('number'); + expect(metrics[0].threshold).toBe(DEPENDABOT_THRESHOLDS); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.test.ts index 970ae34ed3..0e1d51c10f 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.test.ts @@ -135,7 +135,7 @@ describe('FilecheckMetricProvider', () => { ); expect(provider).toBeDefined(); - expect(provider?.getMetricIds()).toEqual([ + expect(provider?.getMetrics().map(m => m.id)).toEqual([ 'filecheck.readme', 'filecheck.license', ]); @@ -254,22 +254,17 @@ describe('FilecheckMetricProvider', () => { expect(provider?.getProviderDatasourceId()).toBe('filecheck'); }); - it('should return correct metric type', () => { - expect(provider?.getMetricType()).toBe('boolean'); - }); - - it('should return all metric IDs', () => { - expect(provider?.getMetricIds()).toEqual([ + it('should return all metrics with correct type and thresholds', () => { + const metrics = provider?.getMetrics(); + expect(metrics?.map(m => m.id)).toEqual([ 'filecheck.readme', 'filecheck.codeowners', 'filecheck.dockerfile', ]); - }); - - it('should return default file check thresholds', () => { - expect(provider?.getMetricThresholds()).toEqual( - DEFAULT_FILECHECK_THRESHOLDS, - ); + metrics?.forEach(m => { + expect(m.type).toBe('boolean'); + expect(m.threshold).toEqual(DEFAULT_FILECHECK_THRESHOLDS); + }); }); it('should return correct catalog filter', () => { @@ -288,6 +283,7 @@ describe('FilecheckMetricProvider', () => { title: 'File: README.md', description: 'Checks if README.md exists in the repository.', type: 'boolean', + threshold: DEFAULT_FILECHECK_THRESHOLDS, history: true, }); expect(metrics?.[1]).toEqual({ @@ -295,18 +291,7 @@ describe('FilecheckMetricProvider', () => { title: 'File: CODEOWNERS', description: 'Checks if CODEOWNERS exists in the repository.', type: 'boolean', - history: true, - }); - }); - - it('should return first metric for backward compatibility via getMetric()', () => { - const metric = provider?.getMetric(); - - expect(metric).toEqual({ - id: 'filecheck.readme', - title: 'File: README.md', - description: 'Checks if README.md exists in the repository.', - type: 'boolean', + threshold: DEFAULT_FILECHECK_THRESHOLDS, history: true, }); }); @@ -398,53 +383,6 @@ describe('FilecheckMetricProvider', () => { ); }); - it('should return first metric result for legacy calculateMetric()', async () => { - const existingFiles = new Set(['README.md']); - const mockUrlReader = createMockUrlReader(existingFiles); - - const config = new ConfigReader({ - scorecard: { - plugins: { - filecheck: { - files: { readme: 'README.md', license: 'LICENSE' }, - }, - }, - }, - }); - const provider = createFilecheckMetricProvider( - config, - mockUrlReader, - createMockCacheService(), - ); - - const result = await provider?.calculateMetric(mockEntity); - - expect(result).toBe(true); - }); - - it('should return false when metric result is not found in legacy calculateMetric()', async () => { - const mockUrlReader = createMockUrlReader(new Set()); - - const config = new ConfigReader({ - scorecard: { - plugins: { - filecheck: { - files: { readme: 'README.md' }, - }, - }, - }, - }); - const provider = createFilecheckMetricProvider( - config, - mockUrlReader, - createMockCacheService(), - ); - - const result = await provider?.calculateMetric(mockEntity); - - expect(result).toBe(false); - }); - it('should handle bare repo source locations without branch ref', async () => { const { getEntitySourceLocation } = jest.requireMock( '@backstage/catalog-model', diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.ts index aa0a669ff7..3570aa7d13 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.ts @@ -16,10 +16,7 @@ import type { Entity } from '@backstage/catalog-model'; import { CATALOG_FILTER_EXISTS } from '@backstage/catalog-client'; -import { - Metric, - ThresholdConfig, -} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecard-node'; import { FilecheckClient } from '../clients/FilecheckClient'; import { @@ -44,32 +41,17 @@ export class FilecheckMetricProvider implements MetricProvider<'boolean'> { return 'filecheck'; } - getMetricIds(): string[] { - return this.filesConfig.files.map(f => `filecheck.${f.id}`); - } - - getMetricType(): 'boolean' { - return 'boolean'; - } - - getMetric(): Metric<'boolean'> { - return this.getMetrics()[0]; - } - getMetrics(): Metric<'boolean'>[] { return this.filesConfig.files.map(f => ({ id: `filecheck.${f.id}`, title: `File: ${f.path}`, description: `Checks if ${f.path} exists in the repository.`, type: 'boolean' as const, + threshold: DEFAULT_FILECHECK_THRESHOLDS, history: true, })); } - getMetricThresholds(): ThresholdConfig { - return DEFAULT_FILECHECK_THRESHOLDS; - } - getCatalogFilter(): Record { return { kind: 'component', @@ -78,12 +60,6 @@ export class FilecheckMetricProvider implements MetricProvider<'boolean'> { }; } - async calculateMetric(entity: Entity): Promise { - const results = await this.calculateMetrics(entity); - const firstId = this.getMetricIds()[0]; - return results.get(firstId) ?? false; - } - async calculateMetrics(entity: Entity): Promise> { const filePaths = this.filesConfig.files.map(f => f.path); const existsMap = await this.client.checkFiles(entity, filePaths); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.test.ts index 82c20f2cb2..d1fda905fe 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.test.ts @@ -31,14 +31,15 @@ jest.mock('../github/GithubClient'); describe('GithubOpenPRsProvider', () => { describe('fromConfig', () => { - it('should create provider with default thresholds', () => { + it('should create provider with default thresholds on metric', () => { const provider = GithubOpenPRsProvider.fromConfig(new ConfigReader({})); - - expect(provider.getMetricThresholds()).toEqual(DEFAULT_NUMBER_THRESHOLDS); + const metrics = provider.getMetrics(); + expect(metrics).toHaveLength(1); + expect(metrics[0].threshold).toEqual(DEFAULT_NUMBER_THRESHOLDS); }); }); - describe('calculateMetric', () => { + describe('calculateMetrics', () => { let provider: GithubOpenPRsProvider; const mockedGithubClient = GithubClient as jest.MockedClass< typeof GithubClient @@ -66,9 +67,9 @@ describe('GithubOpenPRsProvider', () => { }, }; - const result = await provider.calculateMetric(mockEntity); + const results = await provider.calculateMetrics(mockEntity); - expect(result).toBe(42); + expect(results.get('github.open_prs')).toBe(42); expect( mockedGithubClientInstance.getOpenPullRequestsCount, ).toHaveBeenCalledWith('https://github.com/org/orgRepo/tree/main/', { diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.ts index 1263392fac..5074b79fc9 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.ts @@ -20,7 +20,6 @@ import { CATALOG_FILTER_EXISTS } from '@backstage/catalog-client'; import { DEFAULT_NUMBER_THRESHOLDS, Metric, - ThresholdConfig, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecard-node'; import { GithubClient } from '../github/GithubClient'; @@ -41,23 +40,18 @@ export class GithubOpenPRsProvider implements MetricProvider<'number'> { return 'github.open_prs'; } - getMetricType(): 'number' { - return 'number'; - } - - getMetric(): Metric<'number'> { - return { - id: this.getProviderId(), - title: 'GitHub open PRs', - description: - 'Current count of open Pull Requests for a given GitHub repository.', - type: this.getMetricType(), - history: true, - }; - } - - getMetricThresholds(): ThresholdConfig { - return DEFAULT_NUMBER_THRESHOLDS; + getMetrics(): Metric<'number'>[] { + return [ + { + id: this.getProviderId(), + title: 'GitHub open PRs', + description: + 'Current count of open Pull Requests for a given GitHub repository.', + type: 'number', + threshold: DEFAULT_NUMBER_THRESHOLDS, + history: true, + }, + ]; } getCatalogFilter(): Record { @@ -70,7 +64,7 @@ export class GithubOpenPRsProvider implements MetricProvider<'number'> { return new GithubOpenPRsProvider(config); } - async calculateMetric(entity: Entity): Promise { + async calculateMetrics(entity: Entity): Promise> { const repository = getRepositoryInformationFromEntity(entity); const { target } = getEntitySourceLocation(entity); @@ -79,6 +73,8 @@ export class GithubOpenPRsProvider implements MetricProvider<'number'> { repository, ); - return result; + const results = new Map(); + results.set(this.getProviderId(), result); + return results; } } diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.test.ts index 792f8dd4af..0d114f46d9 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.test.ts @@ -16,10 +16,7 @@ import type { Config } from '@backstage/config'; import type { Entity } from '@backstage/catalog-model'; -import { - DEFAULT_NUMBER_THRESHOLDS, - Metric, -} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import { DEFAULT_NUMBER_THRESHOLDS } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { JiraOpenIssuesProvider } from './JiraOpenIssuesProvider'; import { JiraClientFactory } from '../clients/JiraClientFactory'; import { JiraClient } from '../clients/base'; @@ -93,58 +90,25 @@ describe('JiraOpenIssuesProvider', () => { }); }); - describe('getMetricType', () => { - it('should return "number"', () => { - const provider = JiraOpenIssuesProvider.fromConfig( - mockConfig, - mockAuthOptions, - ); - expect(provider.getMetricType()).toEqual('number'); - }); - }); - - describe('getMetric', () => { - let getMetricResult: Metric<'number'>; - - beforeEach(() => { - jest.spyOn(JiraOpenIssuesProvider.prototype, 'getProviderId'); - jest.spyOn(JiraOpenIssuesProvider.prototype, 'getMetricType'); - + describe('getMetrics', () => { + it('should return correct metric metadata with threshold', () => { const provider = JiraOpenIssuesProvider.fromConfig( mockConfig, mockAuthOptions, ); - getMetricResult = provider.getMetric(); - }); + const metrics = provider.getMetrics(); - it('should return correct metric metadata', () => { - expect(getMetricResult).toEqual({ + expect(metrics).toHaveLength(1); + expect(metrics[0]).toEqual({ id: 'jira.open_issues', title: 'Jira open blocking tickets', description: 'Highlights the number of issues that are currently open in Jira.', type: 'number', + threshold: DEFAULT_NUMBER_THRESHOLDS, history: true, }); }); - - it('should call getProviderId', () => { - expect(JiraOpenIssuesProvider.prototype.getProviderId).toHaveBeenCalled(); - }); - - it('should call getMetricType', () => { - expect(JiraOpenIssuesProvider.prototype.getMetricType).toHaveBeenCalled(); - }); - }); - - describe('getMetricThresholds', () => { - it('should return default provider thresholds', () => { - const provider = JiraOpenIssuesProvider.fromConfig( - mockConfig, - mockAuthOptions, - ); - expect(provider.getMetricThresholds()).toEqual(DEFAULT_NUMBER_THRESHOLDS); - }); }); describe('supportsEntity', () => { @@ -168,13 +132,15 @@ describe('JiraOpenIssuesProvider', () => { }); describe('fromConfig', () => { - it('should create provider with default config when thresholds are not configured', () => { + it('should create provider with default thresholds on metric', () => { const provider = JiraOpenIssuesProvider.fromConfig( mockConfig, mockAuthOptions, ); - expect(provider.getMetricThresholds()).toEqual(DEFAULT_NUMBER_THRESHOLDS); + expect(provider.getMetrics()[0].threshold).toEqual( + DEFAULT_NUMBER_THRESHOLDS, + ); }); it('should create provider with proxy connection strategy when proxy path is configured', () => { @@ -203,7 +169,7 @@ describe('JiraOpenIssuesProvider', () => { }); }); - describe('calculateMetric', () => { + describe('calculateMetrics', () => { it('should return the count of open issues when Jira client processed successfully', async () => { mockJiraClient.getCountOpenIssues.mockResolvedValue(5); @@ -211,9 +177,9 @@ describe('JiraOpenIssuesProvider', () => { mockConfig, mockAuthOptions, ); - const result = await provider.calculateMetric(mockEntity); + const results = await provider.calculateMetrics(mockEntity); - expect(result).toBe(5); + expect(results.get('jira.open_issues')).toBe(5); expect(mockJiraClient.getCountOpenIssues).toHaveBeenCalledWith( mockEntity, ); @@ -231,7 +197,7 @@ describe('JiraOpenIssuesProvider', () => { mockConfig, mockAuthOptions, ); - await expect(provider.calculateMetric(mockEntity)).rejects.toThrow( + await expect(provider.calculateMetrics(mockEntity)).rejects.toThrow( 'Jira API error', ); expect(mockJiraClient.getCountOpenIssues).toHaveBeenCalledWith( diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.ts index 6cdbbbc786..031968ab88 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.ts @@ -20,7 +20,6 @@ import { JIRA_CONFIG_PATH } from '../constants'; import { DEFAULT_NUMBER_THRESHOLDS, Metric, - ThresholdConfig, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecard-node'; import { JiraClient } from '../clients/base'; @@ -47,10 +46,6 @@ export class JiraOpenIssuesProvider implements MetricProvider<'number'> { this.jiraClient = JiraClientFactory.create(config, connectionStrategy); } - getMetricThresholds(): ThresholdConfig { - return DEFAULT_NUMBER_THRESHOLDS; - } - getCatalogFilter(): Record { return { 'metadata.annotations.jira/project-key': CATALOG_FILTER_EXISTS, @@ -65,19 +60,18 @@ export class JiraOpenIssuesProvider implements MetricProvider<'number'> { return 'jira.open_issues'; } - getMetricType(): 'number' { - return 'number'; - } - - getMetric(): Metric<'number'> { - return { - id: this.getProviderId(), - title: 'Jira open blocking tickets', - description: - 'Highlights the number of issues that are currently open in Jira.', - type: this.getMetricType(), - history: true, - }; + getMetrics(): Metric<'number'>[] { + return [ + { + id: this.getProviderId(), + title: 'Jira open blocking tickets', + description: + 'Highlights the number of issues that are currently open in Jira.', + type: 'number', + threshold: DEFAULT_NUMBER_THRESHOLDS, + history: true, + }, + ]; } supportsEntity(entity: Entity): boolean { @@ -113,7 +107,10 @@ export class JiraOpenIssuesProvider implements MetricProvider<'number'> { return new JiraOpenIssuesProvider(config, connectionStrategy); } - async calculateMetric(entity: Entity): Promise { - return this.jiraClient.getCountOpenIssues(entity); + async calculateMetrics(entity: Entity): Promise> { + const value = await this.jiraClient.getCountOpenIssues(entity); + const results = new Map(); + results.set(this.getProviderId(), value); + return results; } } diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.test.ts index 0e67fabb46..9279db0316 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.test.ts @@ -85,25 +85,18 @@ describe('OpenSSFMetricProvider', () => { expect(provider.getProviderDatasourceId()).toBe('openssf'); }); - it('returns number as metric type', () => { + it('returns metrics with correct type, threshold, and history', () => { const provider = new OpenSSFMetricProvider(maintainedConfig); - expect(provider.getMetricType()).toBe('number'); - }); - - it('returns metric descriptor with history enabled', () => { - const provider = new OpenSSFMetricProvider(maintainedConfig); - const metric = provider.getMetric(); + const metrics = provider.getMetrics(); + expect(metrics).toHaveLength(1); + const metric = metrics[0]; expect(metric.id).toBe('openssf.maintained'); expect(metric.title).toBe('OpenSSF Maintained'); expect(metric.type).toBe('number'); + expect(metric.threshold).toEqual(OPENSSF_THRESHOLDS); expect(metric.history).toBe(true); }); - it('returns configured thresholds', () => { - const provider = new OpenSSFMetricProvider(maintainedConfig); - expect(provider.getMetricThresholds()).toEqual(OPENSSF_THRESHOLDS); - }); - it('requires openssf/scorecard-location annotation in catalog filter', () => { const provider = new OpenSSFMetricProvider(maintainedConfig); expect(provider.getCatalogFilter()).toEqual({ @@ -113,7 +106,7 @@ describe('OpenSSFMetricProvider', () => { }); }); - describe('calculateMetric', () => { + describe('calculateMetrics', () => { it('returns the score for the configured check', async () => { (globalThis.fetch as jest.Mock).mockResolvedValue({ ok: true, @@ -135,9 +128,9 @@ describe('OpenSSFMetricProvider', () => { }); const provider = new OpenSSFMetricProvider(maintainedConfig); - const result = await provider.calculateMetric(entity); + const results = await provider.calculateMetrics(entity); - expect(result).toBe(8); + expect(results.get('openssf.maintained')).toBe(8); expect(fetch).toHaveBeenCalledWith(scorecardLocation, expect.any(Object)); }); @@ -148,7 +141,7 @@ describe('OpenSSFMetricProvider', () => { .spyOn((provider as any).openSSFClient, 'getScorecard') .mockRejectedValue(propagatedError); - await expect(provider.calculateMetric(entity)).rejects.toBe( + await expect(provider.calculateMetrics(entity)).rejects.toBe( propagatedError, ); expect(getScorecardSpy).toHaveBeenCalledWith(entity); @@ -177,7 +170,7 @@ describe('OpenSSFMetricProvider', () => { const provider = new OpenSSFMetricProvider(maintainedConfig); - await expect(provider.calculateMetric(entity)).rejects.toThrow( + await expect(provider.calculateMetrics(entity)).rejects.toThrow( "OpenSSF check 'Maintained' not found in scorecard", ); }); @@ -206,7 +199,7 @@ describe('OpenSSFMetricProvider', () => { const provider = new OpenSSFMetricProvider(maintainedConfig); - await expect(provider.calculateMetric(entity)).rejects.toThrow( + await expect(provider.calculateMetrics(entity)).rejects.toThrow( `OpenSSF check 'Maintained' has invalid score ${invalidScore}`, ); }, @@ -235,7 +228,7 @@ describe('OpenSSFMetricProvider', () => { expect(providerIds).toEqual(expectedProviderIds); providers.forEach(provider => { expect(provider.getProviderDatasourceId()).toBe('openssf'); - expect(provider.getMetricThresholds()).toEqual(OPENSSF_THRESHOLDS); + expect(provider.getMetrics()[0].threshold).toEqual(OPENSSF_THRESHOLDS); }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.ts index b60d016acc..93912c21d5 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-openssf/src/metricProviders/OpenSSFMetricProvider.ts @@ -17,10 +17,7 @@ import { CATALOG_FILTER_EXISTS } from '@backstage/catalog-client'; import { type Entity } from '@backstage/catalog-model'; -import { - Metric, - ThresholdConfig, -} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecard-node'; import { OpenSSFClient } from '../clients/OpenSSFClient'; @@ -61,22 +58,17 @@ export class OpenSSFMetricProvider implements MetricProvider<'number'> { return `openssf.${normalizedName}`; } - getMetricType(): 'number' { - return 'number'; - } - - getMetric(): Metric<'number'> { - return { - id: this.getProviderId(), - title: this.getMetricDisplayTitle(), - description: this.getMetricDescription(), - type: this.getMetricType(), - history: true, - }; - } - - getMetricThresholds(): ThresholdConfig { - return OPENSSF_THRESHOLDS; + getMetrics(): Metric<'number'>[] { + return [ + { + id: this.getProviderId(), + title: this.getMetricDisplayTitle(), + description: this.getMetricDescription(), + type: 'number', + threshold: OPENSSF_THRESHOLDS, + history: true, + }, + ]; } getCatalogFilter(): Record { @@ -85,7 +77,7 @@ export class OpenSSFMetricProvider implements MetricProvider<'number'> { }; } - async calculateMetric(entity: Entity): Promise { + async calculateMetrics(entity: Entity): Promise> { const scorecard = await this.openSSFClient.getScorecard(entity); const metricName = this.getMetricName(); @@ -98,7 +90,10 @@ export class OpenSSFMetricProvider implements MetricProvider<'number'> { `OpenSSF check '${metricName}' has invalid score ${metric.score}`, ); } - return metric.score; + + const results = new Map(); + results.set(this.getProviderId(), metric.score); + return results; } } diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.test.ts index 9d8d232a1b..c767c75ff5 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.test.ts @@ -104,20 +104,14 @@ describe('SonarQubeBasicMetricProvider', () => { ); }); - describe('getMetricType', () => { + describe('getMetrics', () => { it.each(providers)( - 'should return the metric type given at construction for $type metric type', + 'should return metrics with correct type and thresholds for $type metric type', ({ provider, type }) => { - expect(provider.getMetricType()).toBe(type); - }, - ); - }); - - describe('getMetricThresholds', () => { - it.each(providers)( - 'should return the thresholds from the constructor for $type metric type', - ({ provider }) => { - expect(provider.getMetricThresholds()).toBe(emptyThresholds); + const metrics = provider.getMetrics(); + expect(metrics).toHaveLength(1); + expect(metrics[0].type).toBe(type); + expect(metrics[0].threshold).toBe(emptyThresholds); }, ); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.ts index 54fe39622a..f3f7e4700e 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBasicMetricProvider.ts @@ -43,23 +43,18 @@ export class SonarQubeBasicMetricProvider { return SONARQUBE_METRIC_CONFIG[this.metricId].id; } - getMetricType(): T { - return this.metricType; - } - - getMetric(): Metric { + getMetrics(): Metric[] { const meta = SONARQUBE_METRIC_CONFIG[this.metricId]; - return { - id: meta.id, - title: meta.title, - description: meta.description, - type: this.getMetricType(), - history: true, - }; - } - - getMetricThresholds(): ThresholdConfig { - return this.thresholds; + return [ + { + id: meta.id, + title: meta.title, + description: meta.description, + type: this.metricType, + threshold: this.thresholds, + history: true, + }, + ]; } getCatalogFilter(): Record { diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.test.ts index 29507a9690..7b98b23600 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.test.ts @@ -47,19 +47,21 @@ function entity(projectKey = 'my-project'): Entity { } describe('SonarQubeBooleanMetricProvider', () => { - describe('getMetricThresholds', () => { - it('should create provider with default thresholds', () => { + describe('getMetrics', () => { + it('should create provider with default thresholds on metric', () => { const provider = SonarQubeBooleanMetricProvider.fromConfig( mockConfig, mockLogger, 'quality_gate', ); - expect(provider.getMetricThresholds()).toBeDefined(); - expect(provider.getMetricThresholds().rules).toHaveLength(2); + const metrics = provider.getMetrics(); + expect(metrics).toHaveLength(1); + expect(metrics[0].threshold).toBeDefined(); + expect(metrics[0].threshold.rules).toHaveLength(2); }); }); - describe('calculateMetric', () => { + describe('calculateMetrics', () => { it('should return true when quality gate passes', async () => { mockGetQualityGateStatus.mockResolvedValue(true); const provider = SonarQubeBooleanMetricProvider.fromConfig( @@ -68,9 +70,9 @@ describe('SonarQubeBooleanMetricProvider', () => { 'quality_gate', ); - const result = await provider.calculateMetric(entity()); + const results = await provider.calculateMetrics(entity()); - expect(result).toBe(true); + expect(results.get(provider.getProviderId())).toBe(true); expect(mockGetQualityGateStatus).toHaveBeenCalledWith( 'my-project', undefined, @@ -85,9 +87,9 @@ describe('SonarQubeBooleanMetricProvider', () => { 'quality_gate', ); - const result = await provider.calculateMetric(entity()); + const results = await provider.calculateMetrics(entity()); - expect(result).toBe(false); + expect(results.get(provider.getProviderId())).toBe(false); }); it('should pass instanceName when annotation has instance prefix', async () => { @@ -98,7 +100,7 @@ describe('SonarQubeBooleanMetricProvider', () => { 'quality_gate', ); - await provider.calculateMetric(entity('internal/my-project')); + await provider.calculateMetrics(entity('internal/my-project')); expect(mockGetQualityGateStatus).toHaveBeenCalledWith( 'my-project', @@ -115,7 +117,7 @@ describe('SonarQubeBooleanMetricProvider', () => { const e = entity(); delete e.metadata.annotations!['sonarqube.org/project-key']; - await expect(provider.calculateMetric(e)).rejects.toThrow( + await expect(provider.calculateMetrics(e)).rejects.toThrow( "Missing annotation 'sonarqube.org/project-key'", ); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.ts index 725a388c63..cd8a43baf9 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeBooleanMetricProvider.ts @@ -41,12 +41,18 @@ export class SonarQubeBooleanMetricProvider super(client, metricId, thresholds, 'boolean'); } - async calculateMetric(entity: Entity): Promise { + async calculateMetrics(entity: Entity): Promise> { const { instanceName, projectKey } = parseProjectKeyAnnotation(entity); const mapping = SONARQUBE_API_METRIC_KEYS[this.metricId]; if ('useQualityGateApi' in mapping) { - return this.client.getQualityGateStatus(projectKey, instanceName); + const value = await this.client.getQualityGateStatus( + projectKey, + instanceName, + ); + const results = new Map(); + results.set(this.getProviderId(), value); + return results; } throw new Error(`Unsupported metric ID: ${this.metricId}`); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeMetricProviderFactory.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeMetricProviderFactory.test.ts index 967a648355..446dfdb7ff 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeMetricProviderFactory.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeMetricProviderFactory.test.ts @@ -32,7 +32,7 @@ describe('createMetricProvider', () => { ); expect(provider.getProviderId()).toBe('sonarqube.quality_gate'); expect(provider.getProviderDatasourceId()).toBe('sonarqube'); - expect(provider.getMetricType()).toBe('boolean'); + expect(provider.getMetrics()[0].type).toBe('boolean'); }); it('returns a number provider for open_issues', () => { @@ -42,7 +42,7 @@ describe('createMetricProvider', () => { 'open_issues', ); expect(provider.getProviderId()).toBe('sonarqube.open_issues'); - expect(provider.getMetricType()).toBe('number'); + expect(provider.getMetrics()[0].type).toBe('number'); }); it('returns a number provider for security_rating', () => { @@ -52,7 +52,7 @@ describe('createMetricProvider', () => { 'security_rating', ); expect(provider.getProviderId()).toBe('sonarqube.security_rating'); - expect(provider.getMetricType()).toBe('number'); + expect(provider.getMetrics()[0].type).toBe('number'); }); it('returns a number provider for security_issues', () => { @@ -62,7 +62,7 @@ describe('createMetricProvider', () => { 'security_issues', ); expect(provider.getProviderId()).toBe('sonarqube.security_issues'); - expect(provider.getMetricType()).toBe('number'); + expect(provider.getMetrics()[0].type).toBe('number'); }); }); @@ -94,7 +94,7 @@ describe('fromConfig', () => { mockConfig, mockLogger, ); - const types = providers.map(p => p.getMetricType()); + const types = providers.map(p => p.getMetrics()[0].type); expect(types.filter(t => t === 'boolean')).toHaveLength(1); expect(types.filter(t => t === 'number')).toHaveLength(11); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.test.ts b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.test.ts index a7d4a98209..b08cf637ed 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.test.ts @@ -49,19 +49,21 @@ function entity(projectKey = 'my-project'): Entity { } describe('SonarQubeNumberMetricProvider', () => { - describe('getMetricThresholds', () => { - it('should create provider with default thresholds', () => { + describe('getMetrics', () => { + it('should create provider with default thresholds on metric', () => { const provider = SonarQubeNumberMetricProvider.fromConfig( mockConfig, mockLogger, 'open_issues', ); - expect(provider.getMetricThresholds()).toBeDefined(); - expect(provider.getMetricThresholds().rules).toBeDefined(); + const metrics = provider.getMetrics(); + expect(metrics).toHaveLength(1); + expect(metrics[0].threshold).toBeDefined(); + expect(metrics[0].threshold.rules).toBeDefined(); }); }); - describe('calculateMetric', () => { + describe('calculateMetrics', () => { it('should call getOpenIssuesCount for open_issues metric', async () => { mockGetOpenIssuesCount.mockResolvedValue(42); const provider = SonarQubeNumberMetricProvider.fromConfig( @@ -70,9 +72,9 @@ describe('SonarQubeNumberMetricProvider', () => { 'open_issues', ); - const result = await provider.calculateMetric(entity()); + const results = await provider.calculateMetrics(entity()); - expect(result).toBe(42); + expect(results.get(provider.getProviderId())).toBe(42); expect(mockGetOpenIssuesCount).toHaveBeenCalledWith( 'my-project', undefined, @@ -101,9 +103,9 @@ describe('SonarQubeNumberMetricProvider', () => { metricId, ); - const result = await provider.calculateMetric(entity()); + const results = await provider.calculateMetrics(entity()); - expect(result).toBe(value); + expect(results.get(provider.getProviderId())).toBe(value); expect(mockGetMeasures).toHaveBeenCalledWith( 'my-project', [apiKey], @@ -121,7 +123,7 @@ describe('SonarQubeNumberMetricProvider', () => { 'open_issues', ); - await provider.calculateMetric(entity('internal/my-project')); + await provider.calculateMetrics(entity('internal/my-project')); expect(mockGetOpenIssuesCount).toHaveBeenCalledWith( 'my-project', @@ -138,7 +140,7 @@ describe('SonarQubeNumberMetricProvider', () => { const e = entity(); delete e.metadata.annotations!['sonarqube.org/project-key']; - await expect(provider.calculateMetric(e)).rejects.toThrow( + await expect(provider.calculateMetrics(e)).rejects.toThrow( "Missing annotation 'sonarqube.org/project-key'", ); }); @@ -151,7 +153,8 @@ describe('SonarQubeNumberMetricProvider', () => { 'open_issues', ); - expect(await provider.calculateMetric(entity())).toBe(0); + const results = await provider.calculateMetrics(entity()); + expect(results.get(provider.getProviderId())).toBe(0); }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.ts b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.ts index dfe115dca6..f6677bb270 100644 --- a/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-backend-module-sonarqube/src/metricProviders/SonarQubeNumberMetricProvider.ts @@ -41,24 +41,28 @@ export class SonarQubeNumberMetricProvider super(client, metricId, thresholds, 'number'); } - async calculateMetric(entity: Entity): Promise { + async calculateMetrics(entity: Entity): Promise> { const { instanceName, projectKey } = parseProjectKeyAnnotation(entity); const mapping = SONARQUBE_API_METRIC_KEYS[this.metricId]; - if ('useOpenIssuesApi' in mapping) { - return this.client.getOpenIssuesCount(projectKey, instanceName); - } + let value: number; - if ('apiKey' in mapping) { + if ('useOpenIssuesApi' in mapping) { + value = await this.client.getOpenIssuesCount(projectKey, instanceName); + } else if ('apiKey' in mapping) { const measures = await this.client.getMeasures( projectKey, [mapping.apiKey], instanceName, ); - return measures[mapping.apiKey]; + value = measures[mapping.apiKey]; + } else { + throw new Error(`Unsupported metric ID: ${this.metricId}`); } - throw new Error(`Unsupported metric ID: ${this.metricId}`); + const results = new Map(); + results.set(this.getProviderId(), value); + return results; } static fromConfig( diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts index e067a394b3..6d8fad1bd8 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts @@ -52,14 +52,11 @@ export const buildMockMetricProvidersRegistry = ({ const getMetric = provider || metricsList ? jest.fn().mockImplementation((metricId: string) => { - if (provider?.getMetrics) { + if (provider) { const found = provider.getMetrics().find(m => m.id === metricId); if (found) return found; } - const pMetric = provider?.getMetric(); - if (pMetric && pMetric.id === metricId) return pMetric; - if (metricsList) { const found = metricsList.find(m => m.id === metricId); if (found) return found; diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockProviders.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockProviders.ts index 98eaed269b..c527d1f4bc 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockProviders.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockProviders.ts @@ -49,7 +49,7 @@ abstract class MockMetricProvider protected value: MetricValue, ) {} - abstract getMetricThresholds(): ThresholdConfig; + abstract getDefaultThresholds(): ThresholdConfig; getCatalogFilter(): Record { return MOCK_CATALOG_FILTER; @@ -63,26 +63,28 @@ abstract class MockMetricProvider return this.providerId; } - getMetricType(): T { - return this.metricType; - } - supportsEntity(_: Entity): boolean { return true; } - getMetric(): Metric { - const metric: Metric = { - id: this.providerId, - title: this.title, - description: this.description, - type: this.metricType, - }; - return metric; + getMetrics(): Metric[] { + return [ + { + id: this.providerId, + title: this.title, + description: this.description, + type: this.metricType, + threshold: this.getDefaultThresholds(), + }, + ]; } - async calculateMetric(_entity: Entity): Promise> { - return this.value; + async calculateMetrics( + _entity: Entity, + ): Promise>> { + const results = new Map>(); + results.set(this.providerId, this.value); + return results; } } @@ -96,7 +98,7 @@ export class MockNumberProvider extends MockMetricProvider<'number'> { ) { super('number', providerId, datasourceId, title, description, value); } - getMetricThresholds(): ThresholdConfig { + getDefaultThresholds(): ThresholdConfig { return { rules: [ { key: 'error', expression: '>40' }, @@ -117,7 +119,7 @@ export class MockBooleanProvider extends MockMetricProvider<'boolean'> { ) { super('boolean', providerId, datasourceId, title, description, value); } - getMetricThresholds(): ThresholdConfig { + getDefaultThresholds(): ThresholdConfig { return BOOLEAN_THRESHOLDS; } } @@ -132,6 +134,13 @@ export const githubNumberMetricMetadata = { title: 'Github Number Metric', description: 'Mock number description.', type: 'number' as const, + threshold: { + rules: [ + { key: 'error', expression: '>40' }, + { key: 'warning', expression: '>20' }, + { key: 'success', expression: '<=20' }, + ], + }, }; export const jiraBooleanProvider = new MockBooleanProvider( @@ -144,6 +153,12 @@ export const jiraBooleanMetricMetadata = { title: 'Mock Boolean Metric', description: 'Mock boolean description.', type: 'boolean' as const, + threshold: { + rules: [ + { key: 'success', expression: '==true' }, + { key: 'error', expression: '==false' }, + ], + }, }; /** @@ -168,40 +183,20 @@ export class MockBatchBooleanProvider implements MetricProvider<'boolean'> { return this.providerIdPrefix; } - getMetricType(): 'boolean' { - return 'boolean'; - } - - getMetricIds(): string[] { - return this.metricConfigs.map(c => `${this.providerIdPrefix}.${c.id}`); - } - getMetrics(): Metric<'boolean'>[] { return this.metricConfigs.map(c => ({ id: `${this.providerIdPrefix}.${c.id}`, title: `File: ${c.path}`, description: `Checks if ${c.path} exists.`, type: 'boolean' as const, + threshold: BOOLEAN_THRESHOLDS, })); } - getMetric(): Metric<'boolean'> { - return this.getMetrics()[0]; - } - - getMetricThresholds(): ThresholdConfig { - return BOOLEAN_THRESHOLDS; - } - getCatalogFilter(): Record { return MOCK_CATALOG_FILTER; } - async calculateMetric(_entity: Entity): Promise { - const results = await this.calculateMetrics(_entity); - return results.get(this.getMetricIds()[0]) ?? false; - } - async calculateMetrics(_entity: Entity): Promise> { const results = new Map(); for (const config of this.metricConfigs) { @@ -227,17 +222,20 @@ export const filecheckBatchMetrics = [ title: 'File: README.md', description: 'Checks if README.md exists.', type: 'boolean' as const, + threshold: BOOLEAN_THRESHOLDS, }, { id: 'filecheck.license', title: 'File: LICENSE', description: 'Checks if LICENSE exists.', type: 'boolean' as const, + threshold: BOOLEAN_THRESHOLDS, }, { id: 'filecheck.codeowners', title: 'File: CODEOWNERS', description: 'Checks if CODEOWNERS exists.', type: 'boolean' as const, + threshold: BOOLEAN_THRESHOLDS, }, ]; diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts index 53937d6cae..3412066d5b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/permissions/permissionUtils.test.ts @@ -49,6 +49,7 @@ const createMockMetric = ( title, description: `Description for ${title}`, type: 'number', + threshold: { rules: [] }, history: false, }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts index a616eb76e5..ce22865123 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts @@ -66,48 +66,6 @@ describe('MetricProvidersRegistry', () => { ); }); - it('should throw error when provider ID does not match metric ID', () => { - class InvalidProvider extends MockNumberProvider { - getMetric() { - const metric = super.getMetric(); - return { ...metric, id: 'different.id' }; - } - } - - const invalidProvider = new InvalidProvider( - 'github.test_metric', - 'github', - ); - - expect(() => registry.register(invalidProvider)).toThrow( - new Error( - "Invalid metric provider: metric ID 'github.test_metric' returned by getMetricIds() " + - 'does not have a corresponding metric in getMetrics()', - ), - ); - }); - - it('should throw error when metric type does not match metricType', () => { - class InvalidProvider extends MockNumberProvider { - // @ts-expect-error - put wrong metric type for testing - getMetricType() { - return 'boolean'; - } - } - - const invalidProvider = new InvalidProvider( - 'github.test_metric', - 'github', - ); - - // @ts-expect-error - expect error to be thrown - expect(() => registry.register(invalidProvider)).toThrow( - new Error( - "Invalid metric provider with ID github.test_metric, getMetricType() must match getMetric().type. Expected 'boolean', but got 'number'", - ), - ); - }); - it('should throw error when provider ID does not start with datasource ID', () => { const invalidProvider = new MockNumberProvider( 'invalid_format', @@ -146,10 +104,18 @@ describe('MetricProvidersRegistry', () => { it('should throw error when provider default thresholds are invalid', () => { class InvalidThresholdFormatProvider extends MockNumberProvider { - getMetricThresholds() { - return { - rules: [{ key: 'error', expression: 'Invalid expression' }], - } as any; + getMetrics() { + return [ + { + id: this.getProviderId(), + title: 'Invalid Threshold Metric', + description: 'Test', + type: 'number' as const, + threshold: { + rules: [{ key: 'error', expression: 'Invalid expression' }], + }, + }, + ]; } } @@ -159,7 +125,7 @@ describe('MetricProvidersRegistry', () => { ); expect(() => registry.register(invalidProvider)).toThrow( - 'Invalid default thresholds for metric provider \'github.invalid_threshold_format\'; caused by ThresholdConfigFormatError: Invalid threshold expression: "Invalid expression"', + /Invalid default thresholds for metric provider 'github.invalid_threshold_format'/, ); }); @@ -197,40 +163,8 @@ describe('MetricProvidersRegistry', () => { ); }); - it('should throw error when metric ID from getMetricIds has no corresponding metric', () => { - class InvalidBatchProvider extends MockBatchBooleanProvider { - getMetricIds(): string[] { - return ['filecheck.readme', 'filecheck.nonexistent']; - } - getMetrics() { - return [ - { - id: 'filecheck.readme', - title: 'README', - description: 'README check', - type: 'boolean' as const, - }, - ]; - } - } - - const invalidProvider = new InvalidBatchProvider( - 'filecheck', - 'filecheck', - [], - ); - - expect(() => registry.register(invalidProvider)).toThrow( - "Invalid metric provider: metric ID 'filecheck.nonexistent' returned by getMetricIds() " + - 'does not have a corresponding metric in getMetrics()', - ); - }); - it('should throw error when batch provider metric ID has wrong format', () => { class InvalidBatchProvider extends MockBatchBooleanProvider { - getMetricIds(): string[] { - return ['invalid_format']; - } getMetrics() { return [ { @@ -238,6 +172,12 @@ describe('MetricProvidersRegistry', () => { title: 'Invalid', description: 'Invalid', type: 'boolean' as const, + threshold: { + rules: [ + { key: 'success', expression: '==true' }, + { key: 'error', expression: '==false' }, + ], + }, }, ]; } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts index 7b0c9d3ccd..237e7e57d0 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts @@ -35,31 +35,13 @@ export class MetricProvidersRegistry { register(metricProvider: MetricProvider): void { const providerDatasource = metricProvider.getProviderDatasourceId(); - const metricType = metricProvider.getMetricType(); const providerId = metricProvider.getProviderId(); - // Support both single and batch providers - const metricIds = metricProvider.getMetricIds?.() ?? [providerId]; - const metrics = metricProvider.getMetrics?.() ?? [ - metricProvider.getMetric(), - ]; + const metrics = metricProvider.getMetrics(); + const metricIds = metrics.map(m => m.id); - // Validate: Each metric ID must have a corresponding metric definition - for (const metricId of metricIds) { - const metric = metrics.find(m => m.id === metricId); - if (!metric) { - throw new Error( - `Invalid metric provider: metric ID '${metricId}' returned by getMetricIds() ` + - `does not have a corresponding metric in getMetrics()`, - ); - } - - if (metricType !== metric.type) { - throw new Error( - `Invalid metric provider with ID ${metricId}, getMetricType() must match ` + - `getMetric().type. Expected '${metricType}', but got '${metric.type}'`, - ); - } + for (const metric of metrics) { + const metricId = metric.id; // Validate: Provider ID format (datasource.metric_name) const expectedPrefix = `${providerDatasource}.`; @@ -75,18 +57,15 @@ export class MetricProvidersRegistry { `Metric provider with ID '${metricId}' has already been registered`, ); } - } - try { - validateThresholdsForMetric( - metricProvider.getMetricThresholds(), - metricType, - ); - } catch (error) { - throw new ThresholdConfigFormatError( - `Invalid default thresholds for metric provider '${providerId}'`, - error, - ); + try { + validateThresholdsForMetric(metric.threshold, metric.type); + } catch (error) { + throw new ThresholdConfigFormatError( + `Invalid default thresholds for metric provider '${providerId}', metric '${metricId}'`, + error, + ); + } } for (const metricId of metricIds) { @@ -118,24 +97,30 @@ export class MetricProvidersRegistry { getMetric(metricId: string): Metric { const provider = this.getProvider(metricId); - - // For batch providers, find the specific metric by ID - if (provider.getMetrics) { - const metrics = provider.getMetrics(); - const metric = metrics.find(m => m.id === metricId); - if (metric) { - return metric; - } + const metrics = provider.getMetrics(); + const metric = metrics.find(m => m.id === metricId); + if (metric) { + return metric; } - return provider.getMetric(); + throw new NotFoundError( + `Metric '${metricId}' not found in provider '${provider.getProviderId()}'`, + ); } async calculateMetric( metricId: string, entity: Entity, ): Promise { - return this.getProvider(metricId).calculateMetric(entity); + const provider = this.getProvider(metricId); + const results = await provider.calculateMetrics(entity); + const value = results.get(metricId); + if (value === undefined) { + throw new Error( + `Provider '${provider.getProviderId()}' did not return a value for metric '${metricId}'`, + ); + } + return value; } async calculateMetrics( @@ -167,20 +152,14 @@ export class MetricProvidersRegistry { const provider = this.metricProviders.get(metricId); if (!provider) return undefined; - if (provider.getMetrics) { - const metrics = provider.getMetrics(); - return metrics.find(m => m.id === metricId); - } - - return provider.getMetric(); + const metrics = provider.getMetrics(); + return metrics.find(m => m.id === metricId); }) .filter((m): m is Metric => m !== undefined); } // List all metrics from all providers (deduplicate batch providers) - return this.listProviders().flatMap( - provider => provider.getMetrics?.() ?? [provider.getMetric()], - ); + return this.listProviders().flatMap(provider => provider.getMetrics()); } listMetricsByDatasource(datasourceId: string): Metric[] { @@ -195,8 +174,6 @@ export class MetricProvidersRegistry { .map(id => this.metricProviders.get(id)) .filter((p): p is MetricProvider => p !== undefined); - return [...new Set(providers)].flatMap( - provider => provider.getMetrics?.() ?? [provider.getMetric()], - ); + return [...new Set(providers)].flatMap(provider => provider.getMetrics()); } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts index 391f93eff6..4a678b92e7 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.test.ts @@ -87,7 +87,7 @@ describe('PullMetricsByProviderTask', () => { resolveEntityThresholds: jest.fn().mockReturnValue({ rules: mockThresholdRules, }), - resolveProviderThresholds: jest.fn(), + resolveMetricThresholds: jest.fn(), } as unknown as jest.Mocked; task = new PullMetricsByProviderTask( @@ -194,13 +194,6 @@ describe('PullMetricsByProviderTask', () => { ); }); - it('should get metric type', async () => { - const getMetricTypeSpy = jest.spyOn(mockProvider, 'getMetricType'); - await (task as any).pullProviderMetrics(mockProvider, mockLogger); - - expect(getMetricTypeSpy).toHaveBeenCalledWith(); - }); - it('should query catalog entities', async () => { await (task as any).pullProviderMetrics(mockProvider, mockLogger); @@ -230,23 +223,34 @@ describe('PullMetricsByProviderTask', () => { expect(getOwnServiceCredentialsSpy).toHaveBeenCalledWith(); }); - it('should resolve thresholds for entity/provider', async () => { + it('should resolve thresholds for entity/metric/provider', async () => { await (task as any).pullProviderMetrics(mockProvider, mockLogger); + const metric = mockProvider.getMetrics()[0]; expect( mockThresholdResolver.resolveEntityThresholds, - ).toHaveBeenNthCalledWith(1, mockEntities[0], mockProvider); + ).toHaveBeenNthCalledWith( + 1, + mockEntities[0], + metric, + mockProvider.getProviderId(), + ); expect( mockThresholdResolver.resolveEntityThresholds, - ).toHaveBeenNthCalledWith(2, mockEntities[1], mockProvider); + ).toHaveBeenNthCalledWith( + 2, + mockEntities[1], + metric, + mockProvider.getProviderId(), + ); }); - it('should calculate metric', async () => { - const calculateMetricSpy = jest.spyOn(mockProvider, 'calculateMetric'); + it('should calculate metrics', async () => { + const calculateMetricsSpy = jest.spyOn(mockProvider, 'calculateMetrics'); await (task as any).pullProviderMetrics(mockProvider, mockLogger); - expect(calculateMetricSpy).toHaveBeenNthCalledWith(1, mockEntities[0]); - expect(calculateMetricSpy).toHaveBeenNthCalledWith(2, mockEntities[1]); + expect(calculateMetricsSpy).toHaveBeenNthCalledWith(1, mockEntities[0]); + expect(calculateMetricsSpy).toHaveBeenNthCalledWith(2, mockEntities[1]); }); it('should get threshold evaluator', async () => { @@ -406,14 +410,14 @@ describe('PullMetricsByProviderTask', () => { totalItems: 2, }); - const calculateMetricSpy = jest.spyOn(mockProvider, 'calculateMetric'); + const calculateMetricsSpy = jest.spyOn(mockProvider, 'calculateMetrics'); const createMetricValuesSpy = jest.spyOn( mockDatabaseMetricValues, 'createMetricValues', ); await (task as any).pullProviderMetrics(mockProvider, mockLogger); - expect(calculateMetricSpy).not.toHaveBeenCalled(); + expect(calculateMetricsSpy).not.toHaveBeenCalled(); expect(createMetricValuesSpy).toHaveBeenCalledTimes(1); expect(createMetricValuesSpy).toHaveBeenCalledWith([]); }); @@ -471,16 +475,6 @@ describe('PullMetricsByProviderTask', () => { expect(calculateMetricsSpy).toHaveBeenNthCalledWith(2, mockEntities[1]); }); - it('should not call calculateMetric for batch providers', async () => { - const calculateMetricSpy = jest.spyOn( - mockBatchProvider, - 'calculateMetric', - ); - await (task as any).pullProviderMetrics(mockBatchProvider, mockLogger); - - expect(calculateMetricSpy).not.toHaveBeenCalled(); - }); - it('should create metric values for all metric IDs from batch provider', async () => { const createMetricValuesSpy = jest.spyOn( mockDatabaseMetricValues, diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts index f3091dcf0b..ce43ad4b7b 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/scheduler/tasks/PullMetricsByProviderTask.ts @@ -32,7 +32,10 @@ import { stringifyEntityRef } from '@backstage/catalog-model'; import { DbMetricValueCreate } from '../../database/types'; import { SchedulerOptions, SchedulerTask } from '../types'; import { ThresholdEvaluator } from '../../threshold/ThresholdEvaluator'; -import { MetricValue } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import { + Metric, + MetricValue, +} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { ThresholdResolver } from '../../threshold/ThresholdResolver'; type Options = Pick< @@ -127,9 +130,9 @@ export class PullMetricsByProviderTask implements SchedulerTask { let totalProcessed = 0; let cursor: string | undefined = undefined; - const metricType = provider.getMetricType(); - const isBatchProvider = typeof provider.calculateMetrics === 'function'; - const metricIds = provider.getMetricIds?.() ?? [provider.getProviderId()]; + const metrics = provider.getMetrics(); + const metricsById = new Map(metrics.map(m => [m.id, m])); + const metricIds = metrics.map(m => m.id); try { do { @@ -146,160 +149,99 @@ export class PullMetricsByProviderTask implements SchedulerTask { const batchResults = await Promise.allSettled( entitiesResponse.items.map(async entity => { - // Handle batch providers - if (isBatchProvider && provider.calculateMetrics) { - const entityRef = stringifyEntityRef(entity); - const entityKind = normalizeField(entity.kind); - const entityNamespace = normalizeField(entity.metadata.namespace); - const entityOwner = normalizeOwnerRef(entity?.spec?.owner); - - const enabledMetricIds = metricIds.filter( - metricId => - !isMetricIdDisabled(this.config, metricId, entity, logger), - ); - - if (enabledMetricIds.length === 0) { - return undefined; - } - - try { - const resultsMap = await provider.calculateMetrics(entity); - - return enabledMetricIds.map(metricId => { - if (!resultsMap.has(metricId)) { - return { - catalog_entity_ref: entityRef, - metric_id: metricId, - value: undefined, - timestamp: new Date(), - error_message: `calculateMetrics() did not return an entry for metric '${metricId}'`, - entity_kind: entityKind, - entity_namespace: entityNamespace, - entity_owner: entityOwner, - } as DbMetricValueCreate; - } - - const value = resultsMap.get(metricId) as MetricValue; - - try { - const thresholds = - this.thresholdResolver.resolveEntityThresholds( - entity, - provider, - ); - - const status = - this.thresholdEvaluator.getFirstMatchingThreshold( - value, - metricType, - thresholds, - ); - - return { - catalog_entity_ref: entityRef, - metric_id: metricId, - value, - timestamp: new Date(), - status, - entity_kind: entityKind, - entity_namespace: entityNamespace, - entity_owner: entityOwner, - } as DbMetricValueCreate; - } catch (error) { - return { - catalog_entity_ref: entityRef, - metric_id: metricId, - value, - timestamp: new Date(), - error_message: - error instanceof Error ? error.message : String(error), - entity_kind: entityKind, - entity_namespace: entityNamespace, - entity_owner: entityOwner, - } as DbMetricValueCreate; - } - }); - } catch (error) { - return enabledMetricIds.map( - metricId => - ({ - catalog_entity_ref: entityRef, - metric_id: metricId, - value: undefined, - timestamp: new Date(), - error_message: - error instanceof Error ? error.message : String(error), - entity_kind: entityKind, - entity_namespace: entityNamespace, - entity_owner: entityOwner, - } as DbMetricValueCreate), - ); - } + const entityRef = stringifyEntityRef(entity); + const entityKind = normalizeField(entity.kind); + const entityNamespace = normalizeField(entity.metadata.namespace); + const entityOwner = normalizeOwnerRef(entity?.spec?.owner); + + const enabledMetricIds = metricIds.filter( + metricId => + !isMetricIdDisabled(this.config, metricId, entity, logger), + ); + + if (enabledMetricIds.length === 0) { + return undefined; } - let value: MetricValue | undefined; - try { - if ( - isMetricIdDisabled( - this.config, - provider.getProviderId(), - entity, - logger, - ) - ) { - return undefined; - } - - value = await provider.calculateMetric(entity); - - const thresholds = this.thresholdResolver.resolveEntityThresholds( - entity, - provider, - ); - - const status = this.thresholdEvaluator.getFirstMatchingThreshold( - value, - metricType, - thresholds, - ); - - return { - catalog_entity_ref: stringifyEntityRef(entity), - metric_id: this.providerId, - value, - timestamp: new Date(), - status, - entity_kind: normalizeField(entity.kind), - entity_namespace: normalizeField(entity.metadata.namespace), - entity_owner: normalizeOwnerRef(entity?.spec?.owner), - } as DbMetricValueCreate; + const resultsMap = await provider.calculateMetrics(entity); + + return enabledMetricIds.map(metricId => { + if (!resultsMap.has(metricId)) { + return { + catalog_entity_ref: entityRef, + metric_id: metricId, + value: undefined, + timestamp: new Date(), + error_message: `calculateMetrics() did not return an entry for metric '${metricId}'`, + entity_kind: entityKind, + entity_namespace: entityNamespace, + entity_owner: entityOwner, + } as DbMetricValueCreate; + } + + const value = resultsMap.get(metricId) as MetricValue; + const metric = metricsById.get(metricId)!; + + try { + const thresholds = + this.thresholdResolver.resolveEntityThresholds( + entity, + metric, + provider.getProviderId(), + ); + + const status = + this.thresholdEvaluator.getFirstMatchingThreshold( + value, + metric.type, + thresholds, + ); + + return { + catalog_entity_ref: entityRef, + metric_id: metricId, + value, + timestamp: new Date(), + status, + entity_kind: entityKind, + entity_namespace: entityNamespace, + entity_owner: entityOwner, + } as DbMetricValueCreate; + } catch (error) { + return { + catalog_entity_ref: entityRef, + metric_id: metricId, + value, + timestamp: new Date(), + error_message: + error instanceof Error ? error.message : String(error), + entity_kind: entityKind, + entity_namespace: entityNamespace, + entity_owner: entityOwner, + } as DbMetricValueCreate; + } + }); } catch (error) { - // status is intentionally omitted — a calculation failure produces a NULL status - // in the database, which sorts last when sortBy=status is used - logger.warn( - `Failed to calculate metric for entity ${stringifyEntityRef( - entity, - )}: ${error}`, - error instanceof Error ? error : undefined, + return enabledMetricIds.map( + metricId => + ({ + catalog_entity_ref: entityRef, + metric_id: metricId, + value: undefined, + timestamp: new Date(), + error_message: + error instanceof Error ? error.message : String(error), + entity_kind: entityKind, + entity_namespace: entityNamespace, + entity_owner: entityOwner, + } as DbMetricValueCreate), ); - return { - catalog_entity_ref: stringifyEntityRef(entity), - metric_id: this.providerId, - value, - timestamp: new Date(), - error_message: - error instanceof Error ? error.message : String(error), - entity_kind: normalizeField(entity.kind), - entity_namespace: normalizeField(entity.metadata.namespace), - entity_owner: normalizeOwnerRef(entity?.spec?.owner), - } as DbMetricValueCreate; } }), ).then(promises => promises.reduce((acc, curr) => { if (curr.status === 'fulfilled' && curr.value !== undefined) { - // Batch providers return an array of results, single providers return one result const result = curr.value; if (Array.isArray(result)) { return [...acc, ...result]; diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts index 4617a65531..2d3323de64 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.test.ts @@ -139,7 +139,7 @@ describe('CatalogMetricService', () => { ]); mockedThresholdResolver = { - resolveProviderThresholds: jest.fn(), + resolveMetricThresholds: jest.fn(), resolveEntityThresholds: jest.fn().mockReturnValue({ rules: mockThresholdRules, }), @@ -190,8 +190,8 @@ describe('CatalogMetricService', () => { ); mockedRegistry.getMetric.mockImplementation(id => id === 'github.important_metric' - ? provider.getMetric() - : secondProvider.getMetric(), + ? provider.getMetrics()[0] + : secondProvider.getMetrics()[0], ); const multipleMetrics = [ @@ -324,7 +324,11 @@ describe('CatalogMetricService', () => { expect( mockedThresholdResolver.resolveEntityThresholds, - ).toHaveBeenCalledWith(mockEntity, provider); + ).toHaveBeenCalledWith( + mockEntity, + expect.objectContaining({ id: 'github.important_metric' }), + provider.getProviderId(), + ); }); it('should set threshold error when merge thresholds fails', async () => { @@ -407,10 +411,10 @@ describe('CatalogMetricService', () => { ); expect(resultMetric.metadata).toEqual( expect.objectContaining({ - title: provider.getMetric().title, - description: provider.getMetric().description, - type: provider.getMetric().type, - history: provider.getMetric().history, + title: provider.getMetrics()[0].title, + description: provider.getMetrics()[0].description, + type: provider.getMetrics()[0].type, + history: provider.getMetrics()[0].history, }), ); expect(resultMetric.result).toEqual( @@ -1314,9 +1318,9 @@ describe('CatalogMetricService', () => { ); expect(result.metricMetadata).toEqual({ - title: provider.getMetric().title, - description: provider.getMetric().description, - type: provider.getMetric().type, + title: provider.getMetrics()[0].title, + description: provider.getMetrics()[0].description, + type: provider.getMetrics()[0].type, }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts index b824ec1419..1d91b09b74 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/CatalogMetricService.ts @@ -134,7 +134,8 @@ export class CatalogMetricService { try { thresholds = this.thresholdResolver.resolveEntityThresholds( entity, - provider, + metric, + provider.getProviderId(), ); if (value === null) { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/mappers.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/mappers.test.ts index 5d0a6ef941..8b712fc087 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/mappers.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/mappers.test.ts @@ -30,6 +30,7 @@ describe('AggregatedMetricMapper', () => { title: 'Test Metric', description: 'Test description', type: 'number', + threshold: { rules: [] }, }; describe('toAggregatedMetric', () => { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts index 3c2750b8e3..7cd2f81213 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -738,10 +738,14 @@ describe('createRouter', () => { const { AggregatedMetricMapper: ActualAggregatedMetricMapper } = jest.requireActual('./mappers'); const provider = metricProvidersRegistry.getProvider('github.open_prs'); - const thresholds = provider.getMetricThresholds(); + const metric = metricProvidersRegistry.getMetric('github.open_prs'); + const thresholds = thresholdResolver.resolveMetricThresholds( + metric, + provider.getProviderId(), + ); const emptyAggregatedMetricResult = ActualAggregatedMetricMapper.toAggregatedMetricResult( - provider.getMetric(), + metric, { total: 0, timestamp: emptyAggregatedMetric.timestamp, @@ -874,7 +878,7 @@ describe('createRouter', () => { expect.objectContaining({ total: mockAggregatedMetric.total, timestamp: mockAggregatedMetric.timestamp, - thresholds: batchProvider.getMetricThresholds(), + thresholds: batchProvider.getMetrics()[0].threshold, }), expect.objectContaining({ id: 'filecheck.license', diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 7f27b84d4c..491e9e1ffc 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -183,7 +183,10 @@ export async function createRouter({ await checkEntityAccess(entityRef, req, permissions, httpAuth); } - const thresholds = thresholdResolver.resolveProviderThresholds(provider); + const thresholds = thresholdResolver.resolveMetricThresholds( + metric, + provider.getProviderId(), + ); logger.warn( `Deprecated Scorecard API: GET /metrics/${metricId}/catalog/aggregations is deprecated; use GET /aggregations/:aggregationId (e.g. when the aggregation id matches the metric id, GET /aggregations/${metricId}).`, @@ -305,7 +308,10 @@ export async function createRouter({ ); } - const thresholds = thresholdResolver.resolveProviderThresholds(provider); + const thresholds = thresholdResolver.resolveMetricThresholds( + metric, + provider.getProviderId(), + ); res.json( await aggregationsService.getAggregatedMetricByEntityRefs({ diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.test.ts index f2fd498e0a..391f6a175f 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.test.ts @@ -64,7 +64,12 @@ describe('ThresholdResolver', () => { [provider, new MockNumberProvider('github.other_metric', 'github')], ); - expect(resolver.resolveProviderThresholds(provider)).toEqual({ + expect( + resolver.resolveMetricThresholds( + provider.getMetrics()[0], + provider.getProviderId(), + ), + ).toEqual({ rules: [ { key: 'error', expression: '>40' }, { key: 'warning', expression: '>20' }, @@ -80,7 +85,12 @@ describe('ThresholdResolver', () => { provider, ]); - expect(resolver.resolveProviderThresholds(provider)).toEqual({ + expect( + resolver.resolveMetricThresholds( + provider.getMetrics()[0], + provider.getProviderId(), + ), + ).toEqual({ rules: [ { key: 'error', expression: '>100' }, { key: 'warning', expression: '>50' }, @@ -119,7 +129,12 @@ describe('ThresholdResolver', () => { [new MockNumberProvider('github.other_metric', 'github'), provider], ); - expect(resolver.resolveProviderThresholds(provider)).toEqual({ + expect( + resolver.resolveMetricThresholds( + provider.getMetrics()[0], + provider.getProviderId(), + ), + ).toEqual({ rules: [ { key: 'present', @@ -147,7 +162,13 @@ describe('ThresholdResolver', () => { }) .build(); - expect(resolver.resolveEntityThresholds(entity, provider)).toEqual({ + expect( + resolver.resolveEntityThresholds( + entity, + provider.getMetrics()[0], + provider.getProviderId(), + ), + ).toEqual({ rules: [ { key: 'error', expression: '>40' }, { key: 'warning', expression: '>10' }, @@ -166,7 +187,13 @@ describe('ThresholdResolver', () => { }) .build(); - expect(resolver.resolveEntityThresholds(entity, provider)).toEqual({ + expect( + resolver.resolveEntityThresholds( + entity, + provider.getMetrics()[0], + provider.getProviderId(), + ), + ).toEqual({ rules: [ { key: 'error', expression: '>40' }, { key: 'warning', expression: '>10' }, @@ -187,7 +214,13 @@ describe('ThresholdResolver', () => { }) .build(); - expect(resolver.resolveEntityThresholds(entity, provider)).toEqual({ + expect( + resolver.resolveEntityThresholds( + entity, + provider.getMetrics()[0], + provider.getProviderId(), + ), + ).toEqual({ rules: [ { key: 'error', expression: '>100' }, { key: 'warning', expression: '>10' }, @@ -208,7 +241,13 @@ describe('ThresholdResolver', () => { }) .build(); - expect(resolver.resolveEntityThresholds(entity, provider)).toEqual({ + expect( + resolver.resolveEntityThresholds( + entity, + provider.getMetrics()[0], + provider.getProviderId(), + ), + ).toEqual({ rules: [ { key: 'success', expression: '==false' }, { key: 'error', expression: '==true' }, @@ -229,8 +268,14 @@ describe('ThresholdResolver', () => { const provider = new MockNumberProvider('github.number_metric', 'github'); const resolver = new ThresholdResolver(mockConfig, [provider]); - resolver.resolveProviderThresholds(provider); - resolver.resolveProviderThresholds(provider); + resolver.resolveMetricThresholds( + provider.getMetrics()[0], + provider.getProviderId(), + ); + resolver.resolveMetricThresholds( + provider.getMetrics()[0], + provider.getProviderId(), + ); expect(mockConfig.getOptional).toHaveBeenCalledTimes(1); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.ts b/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.ts index aab841cc6c..abf7eddd81 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/threshold/ThresholdResolver.ts @@ -16,7 +16,10 @@ import type { Config } from '@backstage/config'; import type { Entity } from '@backstage/catalog-model'; -import type { ThresholdConfig } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; +import type { + Metric, + ThresholdConfig, +} from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { getThresholdsFromConfig, type MetricProvider, @@ -32,30 +35,32 @@ export class ThresholdResolver { } } - resolveProviderThresholds(provider: MetricProvider): ThresholdConfig { - return ( - this.configuredThresholds.get(provider.getProviderId()) ?? - provider.getMetricThresholds() - ); + resolveMetricThresholds(metric: Metric, providerId: string): ThresholdConfig { + return this.configuredThresholds.get(providerId) ?? metric.threshold; } resolveEntityThresholds( entity: Entity, - provider: MetricProvider, + metric: Metric, + providerId: string, ): ThresholdConfig { return mergeEntityAndProviderThresholds( entity, - provider, - this.resolveProviderThresholds(provider), + metric, + providerId, + this.resolveMetricThresholds(metric, providerId), ); } private setConfiguredThresholds(provider: MetricProvider): void { const providerId = provider.getProviderId(); + const metrics = provider.getMetrics(); + if (metrics.length === 0) return; + const thresholds = getThresholdsFromConfig( this.config, `scorecard.plugins.${providerId}.thresholds`, - provider.getMetricType(), + metrics[0].type, ); if (thresholds) { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.test.ts index e132434ff0..0be542a4bf 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.test.ts @@ -21,6 +21,7 @@ import { } from '../../__fixtures__/mockProviders'; import { mergeEntityAndProviderThresholds } from '../utils/mergeEntityAndProviderThresholds'; import { ThresholdConfigFormatError } from '@red-hat-developer-hub/backstage-plugin-scorecard-node'; +import { Metric } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; const numberMetricThresholds = { rules: [ @@ -49,6 +50,9 @@ describe('mergeEntityAndProviderThresholds', () => { 'jira', ); + const numberMetric = numberMetricProvider.getMetrics()[0]; + const booleanMetric = booleanMetricProvider.getMetrics()[0]; + beforeEach(() => { entity = { apiVersion: 'backstage.io/v1alpha1', @@ -65,19 +69,21 @@ describe('mergeEntityAndProviderThresholds', () => { }); describe('when entity has no threshold overrides', () => { - it('should return provider thresholds unchanged for number metric', () => { + it('should return metric thresholds unchanged for number metric', () => { const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual(numberMetricThresholds); }); - it('should return provider thresholds unchanged for boolean metric', () => { + it('should return metric thresholds unchanged for boolean metric', () => { const result = mergeEntityAndProviderThresholds( entity, - booleanMetricProvider, + booleanMetric, + booleanMetricProvider.getProviderId(), ); expect(result).toEqual(booleanMetricThresholds); }); @@ -86,7 +92,8 @@ describe('mergeEntityAndProviderThresholds', () => { entity.metadata.annotations = {}; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual(numberMetricThresholds); }); @@ -95,20 +102,22 @@ describe('mergeEntityAndProviderThresholds', () => { entity.metadata = { name: 'test-component' }; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual(numberMetricThresholds); }); }); describe('when entity has valid threshold overrides', () => { - it('should override single provider threshold rule for number metric', () => { + it('should override single metric threshold rule for number metric', () => { entity.metadata.annotations = { 'scorecard.io/github.important_metric.thresholds.rules.error': '>50', }; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual({ @@ -120,7 +129,7 @@ describe('mergeEntityAndProviderThresholds', () => { }); }); - it('should override multiple provider threshold rules for number metric', () => { + it('should override multiple metric threshold rules for number metric', () => { entity.metadata.annotations = { 'scorecard.io/github.important_metric.thresholds.rules.error': '>50', 'scorecard.io/github.important_metric.thresholds.rules.warning': '>30', @@ -128,7 +137,8 @@ describe('mergeEntityAndProviderThresholds', () => { }; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual({ @@ -140,13 +150,14 @@ describe('mergeEntityAndProviderThresholds', () => { }); }); - it('should override provider threshold rule for boolean metric', () => { + it('should override metric threshold rule for boolean metric', () => { entity.metadata.annotations = { 'scorecard.io/jira.boolean_metric.thresholds.rules.success': '!=false', }; const result = mergeEntityAndProviderThresholds( entity, - booleanMetricProvider, + booleanMetric, + booleanMetricProvider.getProviderId(), ); expect(result).toEqual({ @@ -163,7 +174,8 @@ describe('mergeEntityAndProviderThresholds', () => { }; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual(numberMetricThresholds); @@ -177,7 +189,8 @@ describe('mergeEntityAndProviderThresholds', () => { }; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual({ @@ -198,7 +211,8 @@ describe('mergeEntityAndProviderThresholds', () => { }; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result).toEqual({ @@ -210,35 +224,36 @@ describe('mergeEntityAndProviderThresholds', () => { }); }); - it('should preserve color from provider rules when overriding', () => { - class MockColorNumberProvider extends MockNumberProvider { - getMetricThresholds() { - return { - rules: [ - { key: 'low', expression: '<10', color: 'success.main' }, - { - key: 'high', - expression: '10-20', - color: '#FF0000', - icon: 'scorecardWarningStatusIcon', - }, - { key: 'error', expression: '>20' }, - ], - }; - } - } - - const provider = new MockColorNumberProvider( - 'github.custom_metric', - 'github', - ); + it('should preserve color from metric rules when overriding', () => { + const customMetric: Metric<'number'> = { + id: 'github.custom_metric', + title: 'Custom Metric', + description: 'Custom metric description.', + type: 'number', + threshold: { + rules: [ + { key: 'low', expression: '<10', color: 'success.main' }, + { + key: 'high', + expression: '10-20', + color: '#FF0000', + icon: 'scorecardWarningStatusIcon', + }, + { key: 'error', expression: '>20' }, + ], + }, + }; entity.metadata.annotations = { 'scorecard.io/github.custom_metric.thresholds.rules.high': '10-60', 'scorecard.io/github.custom_metric.thresholds.rules.error': '>60', }; - const result = mergeEntityAndProviderThresholds(entity, provider); + const result = mergeEntityAndProviderThresholds( + entity, + customMetric, + 'github.custom_metric', + ); expect(result).toEqual({ rules: [ @@ -255,32 +270,39 @@ describe('mergeEntityAndProviderThresholds', () => { }); it('should throw when merged annotation rules leave a gap on the real line', () => { - class PartitionProvider extends MockNumberProvider { - getMetricThresholds() { - return { - rules: [ - { key: 'success', expression: '<10' }, - { key: 'warning', expression: '10-20' }, - { key: 'error', expression: '>20' }, - ], - }; - } - } - const provider = new PartitionProvider( - 'github.partition_metric', - 'github', - ); + const partitionMetric: Metric<'number'> = { + id: 'github.partition_metric', + title: 'Partition Metric', + description: 'Test', + type: 'number', + threshold: { + rules: [ + { key: 'success', expression: '<10' }, + { key: 'warning', expression: '10-20' }, + { key: 'error', expression: '>20' }, + ], + }, + }; + entity.metadata.annotations = { 'scorecard.io/github.partition_metric.thresholds.rules.warning': '11-20', }; - expect(() => mergeEntityAndProviderThresholds(entity, provider)).toThrow( - ThresholdConfigFormatError, - ); - expect(() => mergeEntityAndProviderThresholds(entity, provider)).toThrow( - /do not cover the entire real line/, - ); + expect(() => + mergeEntityAndProviderThresholds( + entity, + partitionMetric, + 'github.partition_metric', + ), + ).toThrow(ThresholdConfigFormatError); + expect(() => + mergeEntityAndProviderThresholds( + entity, + partitionMetric, + 'github.partition_metric', + ), + ).toThrow(/do not cover the entire real line/); }); }); @@ -289,10 +311,18 @@ describe('mergeEntityAndProviderThresholds', () => { 'scorecard.io/github.important_metric.thresholds.rules.error': 'invalid', }; expect(() => - mergeEntityAndProviderThresholds(entity, numberMetricProvider), + mergeEntityAndProviderThresholds( + entity, + numberMetric, + numberMetricProvider.getProviderId(), + ), ).toThrow(ThresholdConfigFormatError); expect(() => - mergeEntityAndProviderThresholds(entity, numberMetricProvider), + mergeEntityAndProviderThresholds( + entity, + numberMetric, + numberMetricProvider.getProviderId(), + ), ).toThrow( "Invalid threshold annotation 'scorecard.io/github.important_metric.thresholds.rules.error: invalid' in entity 'component:default/test-component'", ); @@ -303,23 +333,32 @@ describe('mergeEntityAndProviderThresholds', () => { 'scorecard.io/jira.boolean_metric.thresholds.rules.success': '>40', }; expect(() => - mergeEntityAndProviderThresholds(entity, booleanMetricProvider), + mergeEntityAndProviderThresholds( + entity, + booleanMetric, + booleanMetricProvider.getProviderId(), + ), ).toThrow(ThresholdConfigFormatError); expect(() => - mergeEntityAndProviderThresholds(entity, booleanMetricProvider), + mergeEntityAndProviderThresholds( + entity, + booleanMetric, + booleanMetricProvider.getProviderId(), + ), ).toThrow( "Invalid threshold annotation 'scorecard.io/jira.boolean_metric.thresholds.rules.success: >40' in entity 'component:default/test-component'", ); }); - it('should preserve order of provider rules when overriding', () => { + it('should preserve order of metric rules when overriding', () => { entity.metadata.annotations = { 'scorecard.io/github.important_metric.thresholds.rules.success': '<=20', 'scorecard.io/github.important_metric.thresholds.rules.error': '>50', }; const result = mergeEntityAndProviderThresholds( entity, - numberMetricProvider, + numberMetric, + numberMetricProvider.getProviderId(), ); expect(result.rules[0]).toEqual({ key: 'error', expression: '>50' }); @@ -328,34 +367,32 @@ describe('mergeEntityAndProviderThresholds', () => { }); it('should throw error when entity has invalid threshold key', () => { - class MockProvider extends MockNumberProvider { - constructor( - providerId: string, - datasourceId: string, - title: string = 'Mock Number Metric', - description: string = 'Mock number description.', - ) { - super(providerId, datasourceId, title, description); - } - - getMetricThresholds() { - return { - rules: [{ key: 'error', expression: '>40' }], - }; - } - } - const mockedProvider = new MockProvider( - 'github.important_metric', - 'github', - ); + const singleRuleMetric: Metric<'number'> = { + id: 'github.important_metric', + title: 'Mock Number Metric', + description: 'Mock number description.', + type: 'number', + threshold: { + rules: [{ key: 'error', expression: '>40' }], + }, + }; + entity.metadata.annotations = { 'scorecard.io/github.important_metric.thresholds.rules.success': '<=10', }; expect(() => - mergeEntityAndProviderThresholds(entity, mockedProvider), + mergeEntityAndProviderThresholds( + entity, + singleRuleMetric, + 'github.important_metric', + ), ).toThrow(ThresholdConfigFormatError); expect(() => - mergeEntityAndProviderThresholds(entity, mockedProvider), + mergeEntityAndProviderThresholds( + entity, + singleRuleMetric, + 'github.important_metric', + ), ).toThrow( 'Unable to override component:default/test-component thresholds by {"key":"success","expression":"<=10"}, metric provider github.important_metric does not support key success', ); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.ts b/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.ts index cfc730507c..e353d86f70 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/utils/mergeEntityAndProviderThresholds.ts @@ -16,11 +16,11 @@ import { stringifyEntityRef, type Entity } from '@backstage/catalog-model'; import type { + Metric, ThresholdConfig, ThresholdRule, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; import { - type MetricProvider, validateThresholdsForMetric, ThresholdConfigFormatError, validateThresholdNumberIntervals, @@ -53,14 +53,14 @@ function parseEntityAnnotationThresholds( export function mergeEntityAndProviderThresholds( entity: Entity, - provider: MetricProvider, + metric: Metric, + providerId: string, baseThresholds?: ThresholdConfig, ): ThresholdConfig { let isRulesMerged = false; - const providerId = provider.getProviderId(); - const providerThresholds = baseThresholds ?? provider.getMetricThresholds(); - const providerMetricType = provider.getMetricType(); + const providerThresholds = baseThresholds ?? metric.threshold; + const metricType = metric.type; const entityAnnotationThresholds = parseEntityAnnotationThresholds( entity, providerId, @@ -81,7 +81,7 @@ export function mergeEntityAndProviderThresholds( const mergedRule: ThresholdRule = { ...mergedRules[foundKey], ...override }; try { - validateThresholdsForMetric({ rules: [mergedRule] }, providerMetricType); + validateThresholdsForMetric({ rules: [mergedRule] }, metricType); if (!isRulesMerged) isRulesMerged = true; } catch (e) { @@ -101,7 +101,7 @@ export function mergeEntityAndProviderThresholds( } if (isRulesMerged) { - validateThresholdNumberIntervals(mergedRules, providerMetricType); + validateThresholdNumberIntervals(mergedRules, metricType); } return { diff --git a/workspaces/scorecard/plugins/scorecard-common/report.api.md b/workspaces/scorecard/plugins/scorecard-common/report.api.md index 213f62c1bd..5a7fe8fc44 100644 --- a/workspaces/scorecard/plugins/scorecard-common/report.api.md +++ b/workspaces/scorecard/plugins/scorecard-common/report.api.md @@ -123,6 +123,7 @@ export type Metric = { title: string; description: string; type: T; + threshold: ThresholdConfig; history?: boolean; }; diff --git a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts index 5032d137b6..7035f93c17 100644 --- a/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts +++ b/workspaces/scorecard/plugins/scorecard-common/src/types/Metric.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { ThresholdResult } from './threshold'; +import { ThresholdConfig, ThresholdResult } from './threshold'; /** * @public @@ -38,6 +38,7 @@ export type Metric = { title: string; description: string; type: T; + threshold: ThresholdConfig; history?: boolean; }; diff --git a/workspaces/scorecard/plugins/scorecard-node/report.api.md b/workspaces/scorecard/plugins/scorecard-node/report.api.md index 0e1d830511..4af6b8bec0 100644 --- a/workspaces/scorecard/plugins/scorecard-node/report.api.md +++ b/workspaces/scorecard/plugins/scorecard-node/report.api.md @@ -32,14 +32,9 @@ export function getThresholdsFromConfig( // @public export interface MetricProvider { - calculateMetric(entity: Entity): Promise>; - calculateMetrics?(entity: Entity): Promise>>; + calculateMetrics(entity: Entity): Promise>>; getCatalogFilter(): Record; - getMetric(): Metric; - getMetricIds?(): string[]; - getMetrics?(): Metric[]; - getMetricThresholds(): ThresholdConfig; - getMetricType(): T; + getMetrics(): Metric[]; getProviderDatasourceId(): string; getProviderId(): string; } diff --git a/workspaces/scorecard/plugins/scorecard-node/src/api/MetricProvider.ts b/workspaces/scorecard/plugins/scorecard-node/src/api/MetricProvider.ts index 445f9ba5a0..ce8740ef62 100644 --- a/workspaces/scorecard/plugins/scorecard-node/src/api/MetricProvider.ts +++ b/workspaces/scorecard/plugins/scorecard-node/src/api/MetricProvider.ts @@ -19,7 +19,6 @@ import { Metric, MetricType, MetricValue, - ThresholdConfig, } from '@red-hat-developer-hub/backstage-plugin-scorecard-common'; /** @@ -38,52 +37,20 @@ export interface MetricProvider { */ getProviderId(): string; /** - * Get the metric type for the metric provider - * @public - */ - getMetricType(): T; - /** - * Get the metric for the metric provider - * @public - */ - getMetric(): Metric; - /** - * Get the default metric thresholds for the metric provider + * Get all metrics this provider exposes. + * Each metric includes its own type and threshold configuration. * @public */ - getMetricThresholds(): ThresholdConfig; + getMetrics(): Metric[]; /** - * Calculate the metric for the metric provider + * Calculate multiple metrics in a single call. + * Returns a map of metric ID to metric value. * @public */ - calculateMetric(entity: Entity): Promise>; + calculateMetrics(entity: Entity): Promise>>; /** * Get the catalog filter for the metric provider * @public */ getCatalogFilter(): Record; - - /** - * Get all metric IDs this provider handles. - * For batch providers that handle multiple metrics. - * Defaults to [getProviderId()] if not implemented. - * @public - */ - getMetricIds?(): string[]; - - /** - * Get all metrics this provider exposes. - * For batch providers that handle multiple metrics. - * Defaults to [getMetric()] if not implemented. - * @public - */ - getMetrics?(): Metric[]; - - /** - * Calculate multiple metrics in a single call. - * For batch providers that can efficiently compute multiple metrics together. - * Defaults to [calculateMetric()] ff not implemented. - * @public - */ - calculateMetrics?(entity: Entity): Promise>>; } diff --git a/workspaces/scorecard/plugins/scorecard/dev/legacy.tsx b/workspaces/scorecard/plugins/scorecard/dev/legacy.tsx index b07c6ad940..2e9c07383b 100644 --- a/workspaces/scorecard/plugins/scorecard/dev/legacy.tsx +++ b/workspaces/scorecard/plugins/scorecard/dev/legacy.tsx @@ -116,6 +116,7 @@ class MockScorecardApi implements ScorecardApi { title: m.metadata.title, description: m.metadata.description, type: m.metadata.type, + threshold: m.result.thresholdResult.definition ?? { rules: [] }, history: m.metadata.history, })); return { metrics: allMetrics }; diff --git a/workspaces/scorecard/plugins/scorecard/dev/mocks.ts b/workspaces/scorecard/plugins/scorecard/dev/mocks.ts index 8bca8dbbe6..63df7b2e06 100644 --- a/workspaces/scorecard/plugins/scorecard/dev/mocks.ts +++ b/workspaces/scorecard/plugins/scorecard/dev/mocks.ts @@ -80,6 +80,7 @@ export class MockScorecardApi implements ScorecardApi { title: m.metadata.title, description: m.metadata.description, type: m.metadata.type, + threshold: m.result.thresholdResult.definition ?? { rules: [] }, history: m.metadata.history, })); return { metrics: allMetrics }; diff --git a/workspaces/scorecard/plugins/scorecard/src/hooks/__tests__/useMetric.test.tsx b/workspaces/scorecard/plugins/scorecard/src/hooks/__tests__/useMetric.test.tsx index b3b1ecfb89..05c5456094 100644 --- a/workspaces/scorecard/plugins/scorecard/src/hooks/__tests__/useMetric.test.tsx +++ b/workspaces/scorecard/plugins/scorecard/src/hooks/__tests__/useMetric.test.tsx @@ -46,6 +46,7 @@ describe('useMetric', () => { description: 'Current count of open Pull Requests for a given GitHub repository.', type: 'number', + threshold: { rules: [] }, }; beforeEach(() => {