Skip to content

feat(#3529): move ThresholdConfig from MetricProvider to individual Metrics #3560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fullsend-ai-coder wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(#3529): move ThresholdConfig from MetricProvider to individual Metrics #3560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 11 additions & 29 deletions .../scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand All @@ -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(
Expand Down Expand Up @@ -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 => {
Expand All @@ -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',
Expand All @@ -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 () => {
Expand All @@ -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(
Expand Down
38 changes: 16 additions & 22 deletions ...ugins/scorecard-backend-module-dependabot/src/metricProviders/DependabotMetricProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<string, string | symbol | (string | symbol)[]> {
Expand Down Expand Up @@ -112,14 +104,16 @@ export class DependabotMetricProvider implements MetricProvider<'number'> {
return { owner, repo };
}

async calculateMetric(entity: Entity): Promise<number> {
async calculateMetrics(entity: Entity): Promise<Map<string, number>> {
const repository = this.getRepository(entity);
const { target } = getEntitySourceLocation(entity);
const alerts = await this.dependabotClient.getAlerts(
target,
repository,
this.severity,
);
return alerts.length;
const results = new Map<string, number>();
results.set(this.getProviderId(), alerts.length);
return results;
}
}
6 changes: 4 additions & 2 deletions ...ard-backend-module-dependabot/src/metricProviders/DependabotMetricProviderFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand Down
82 changes: 10 additions & 72 deletions ...ns/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
]);
Expand Down Expand Up @@ -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', () => {
Expand All @@ -288,25 +283,15 @@ 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({
id: 'filecheck.codeowners',
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,
});
});
Expand Down Expand Up @@ -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',
Expand Down
28 changes: 2 additions & 26 deletions ...plugins/scorecard-backend-module-filecheck/src/metricProviders/FilecheckMetricProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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<string, string | symbol | (string | symbol)[]> {
return {
kind: 'component',
Expand All @@ -78,12 +60,6 @@ export class FilecheckMetricProvider implements MetricProvider<'boolean'> {
};
}

async calculateMetric(entity: Entity): Promise<boolean> {
const results = await this.calculateMetrics(entity);
const firstId = this.getMetricIds()[0];
return results.get(firstId) ?? false;
}

async calculateMetrics(entity: Entity): Promise<Map<string, boolean>> {
const filePaths = this.filesConfig.files.map(f => f.path);
const existsMap = await this.client.checkFiles(entity, filePaths);
Expand Down
13 changes: 7 additions & 6 deletions ...plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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/', {
Expand Down
Loading
Loading