fix(scorecard): forward entity threshold annotation errors (#1428)

* Throw error on invalid annotation

Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>

* Update default thresholds order

Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>

* Fix schema change

Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>

* Introduce supportsEntity

Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>

---------

Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com>

Author: dzemanov

workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.test.ts

@@ -18,6 +18,7 @@ import { ConfigReader } from '@backstage/config';
 import type { Entity } from '@backstage/catalog-model';
 import { GithubOpenPRsProvider } from './GithubOpenPRsProvider';
 import { GithubClient } from '../github/GithubClient';
+import { DEFAULT_NUMBER_THRESHOLDS } from '@red-hat-developer-hub/backstage-plugin-scorecard-common';
 
 jest.mock('@backstage/catalog-model', () => ({
   ...jest.requireActual('@backstage/catalog-model'),
@@ -29,17 +30,47 @@ jest.mock('@backstage/catalog-model', () => ({
 jest.mock('../github/GithubClient');
 
 describe('GithubOpenPRsProvider', () => {
+  describe('supportsEntity', () => {
+    let provider: GithubOpenPRsProvider;
+
+    beforeEach(() => {
+      provider = GithubOpenPRsProvider.fromConfig(new ConfigReader({}));
+    });
+
+    it.each([
+      [
+        'should return true for entity with github.com/project-slug annotation',
+        {
+          'github.com/project-slug': 'org/repo',
+        },
+        true,
+      ],
+      [
+        'should return false for entity without github.com/project-slug annotation',
+        {
+          'some.other/annotation': 'value',
+        },
+        false,
+      ],
+      ['should return false for entity with no annotations', undefined, false],
+    ])('%s', (_, annotations, expected) => {
+      const entity: Entity = {
+        apiVersion: 'backstage.io/v1alpha1',
+        kind: 'Component',
+        metadata: {
+          name: 'test-component',
+          annotations,
+        },
+      };
+      expect(provider.supportsEntity(entity)).toBe(expected);
+    });
+  });
+
   describe('fromConfig', () => {
     it('should create provider with default thresholds when no thresholds are configured', () => {
       const provider = GithubOpenPRsProvider.fromConfig(new ConfigReader({}));
 
-      expect(provider.getMetricThresholds()).toEqual({
-        rules: [
-          { key: 'error', expression: '>50' },
-          { key: 'warning', expression: '10-50' },
-          { key: 'success', expression: '<10' },
-        ],
-      });
+      expect(provider.getMetricThresholds()).toEqual(DEFAULT_NUMBER_THRESHOLDS);
     });
 
     it('should create provider with custom thresholds when configured', () => {

workspaces/scorecard/plugins/scorecard-backend-module-github/src/metricProviders/GithubOpenPRsProvider.ts

@@ -30,6 +30,7 @@ import {
   getHostnameFromEntity,
   getRepositoryInformationFromEntity,
 } from '../github/utils';
+import { GITHUB_PROJECT_ANNOTATION } from '../github/constants';
 
 export class GithubOpenPRsProvider implements MetricProvider<'number'> {
   private readonly thresholds: ThresholdConfig;
@@ -63,6 +64,12 @@ export class GithubOpenPRsProvider implements MetricProvider<'number'> {
     return this.thresholds;
   }
 
+  supportsEntity(entity: Entity): boolean {
+    return (
+      entity.metadata.annotations?.[GITHUB_PROJECT_ANNOTATION] !== undefined
+    );
+  }
+
   static fromConfig(config: Config): GithubOpenPRsProvider {
     const configPath = 'scorecard.plugins.github.open_prs.thresholds';
     const configuredThresholds = config.getOptional(configPath);

workspaces/scorecard/plugins/scorecard-backend-module-jira/src/metricProviders/JiraOpenIssuesProvider.ts

@@ -56,6 +56,10 @@ export class JiraOpenIssuesProvider implements MetricProvider<'number'> {
     return this.thresholds;
   }
 
+  supportsEntity(entity: Entity): boolean {
+    return entity.metadata.annotations?.['jira/project-key'] !== undefined;
+  }
+
   static fromConfig(config: Config): JiraOpenIssuesProvider {
     const configPath = 'scorecard.plugins.jira.open_issues.thresholds';
     const configuredThresholds = config.getOptional(configPath);

workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockProviders.ts

@@ -45,6 +45,10 @@ abstract class MockMetricProvider<T extends MetricType>
     return this.providerId;
   }
 
+  supportsEntity(_: Entity): boolean {
+    return true;
+  }
+
   getMetric(): Metric<T> {
     const metric: Metric<T> = {
       id: this.providerId,

workspaces/scorecard/plugins/scorecard-backend/docs/providers.md

@@ -67,6 +67,11 @@ export class MyMetricProvider implements MetricProvider<'number'> {
     };
   }
 
+  // Determines whether this metric can be calculated for a specific entity (e.g., based on annotations)
+  supportsEntity(_: Entity): boolean {
+    return true;
+  }
+
   // Calculates and returns the metric value
   async calculateMetric(): Promise<number> {
     // TODO: Implement your metric calculation logic here

workspaces/scorecard/plugins/scorecard-backend/docs/thresholds.md

@@ -1,12 +1,12 @@
 # Thresholds Configuration
 
-Thresholds define conditions that determine which category a metric value belongs to ( `error`, `warning`, or `success`). When a metric value matches a threshold condition, it gets assigned to that threshold's category. The Scorecard plugin provides multiple ways to configure thresholds with a flexible priority system.
+Thresholds define conditions that determine which category a metric value belongs to (`success`, `warning`, or `error`). When a metric value matches a threshold condition, it gets assigned to that threshold's category. The Scorecard plugin provides multiple ways to configure thresholds with a flexible priority system.
 
 ## Overview
 
 Thresholds are evaluated in order and the **first matching** threshold rule is applied. Each threshold rule consists of:
 
-- **`key`**: The threshold category (only allowed keys are `error`, `warning`, `success`)
+- **`key`**: The threshold category (only allowed keys are `success`, `warning`, `error`)
 - **`expression`**: The condition that determines if a metric value matches this threshold
 
 ## Threshold Configuration Options
@@ -24,9 +24,9 @@ export class MyMetricProvider implements MetricProvider<'number'> {
   getMetricThresholds(): ThresholdConfig {
     return {
       rules: [
-        { key: 'error', expression: '>50' },
-        { key: 'warning', expression: '10-50' },
         { key: 'success', expression: '<10' },
+        { key: 'warning', expression: '10-50' },
+        { key: 'error', expression: '>50' },
       ],
     };
   }
@@ -81,12 +81,12 @@ scorecard:
       myMetric:
         thresholds:
           rules:
+            - key: success
+              expression: '<10'
+            - key: warning
+              expression: '<=20'
             - key: error
               expression: '>20'
-            - key: warning
-              expression: '>10'
-            - key: success
-              expression: '<=10'
     myOtherDatasource:
       myOtherMetric: ...
 ```
@@ -102,9 +102,9 @@ metadata:
   name: my-service
   annotations:
     # Override specific threshold rules for this entity
+    scorecard.io/myDatasource.myMetric.thresholds.rules.warning: '10-15'
     scorecard.io/myDatasource.myMetric.thresholds.rules.error: '>15'
-    scorecard.io/myDatasource.myMetric.thresholds.rules.warning: '>8'
-    # success threshold will use the default/config value
+    # success threshold will use the default config value
 spec:
   type: service
 ```
@@ -120,7 +120,7 @@ scorecard.io/{providerId}.thresholds.rules.{thresholdKey}: '{expression}'
 Where:
 
 - `{providerId}`: The metric provider ID (e.g., `github.open-prs`)
-- `{thresholdKey}`: The threshold category (e.g., `error`, `warning`, `success`)
+- `{thresholdKey}`: The threshold category (e.g., `success`, `warning`, `error`)
 - `{expression}`: The threshold expression (e.g., `>10`, `==true`, `5-15`)
 
 ## Threshold Priority Order
@@ -143,28 +143,29 @@ Thresholds are applied with the following priority (highest to lowest):
 ```typescript
 // 1. Provider defaults
 providerDefaults: [
-  { key: 'error', expression: '>10' },
-  { key: 'warning', expression: '>5' },
-  { key: 'success', expression: '<=5' }
+  { key: 'success', expression: '<10' },
+  { key: 'warning', expression: '10-50' },
+  { key: 'error', expression: '>50' }
 ]
 
 // 2. App configuration (completely replaces defaults)
 appConfig: [
-  { key: 'error', expression: '>20' },
-  { key: 'warning', expression: '>10' },
-  { key: 'success', expression: '<=10' }
+  { key: 'success', expression: '<10' },
+  { key: 'warning', expression: '10-30' },
+  { key: 'error', expression: '>30' },
 ]
 
 // 3. Entity annotation overrides (merged with app-config)
 annotations: {
-  'scorecard.io/myProvider.thresholds.rules.error': '>25',
+  'scorecard.io/myProviderId.thresholds.rules.warning': '10-25',
+  'scorecard.io/myProviderId.thresholds.rules.error': '>25',
 }
 
 // Final result (annotations merged with app-config)
 finalRules: [
+  { key: 'success', expression: '<10' },   // from app-config
+  { key: 'warning', expression: '10-25' }, // from annotation (overrides app-config)
   { key: 'error', expression: '>25' }, // from annotation (overrides app-config)
-  { key: 'warning', expression: '>10' }, // from app-config
-  { key: 'success', expression: '<=10' }   // from app-config
 ]
 ```
 
@@ -225,27 +226,27 @@ The `ThresholdEvaluator` service processes threshold rules and determines which
 
 ### Key Features
 
-1. **Order-dependent evaluation**: Rules are evaluated in the order they appear
+1. **Order-dependent evaluation**: Rules are evaluated in the order they appear. If provider supports overriding defaults through [app configuration](#App-Configuration-Thresholds), you can change the evaluation order by specifying threshold keys in a different order. Entity annotations cannot alter the evaluation order, which is determined by either the [app configuration](#Provider-Default-Thresholds) or, if not specified, the [default provider configuration](#Provider-Default-Thresholds).
 2. **First-match wins**: Returns the first threshold rule whose condition the value satisfies
 3. **Type-safe**: Validates expressions against metric types
-4. **Error handling**: Graceful handling of invalid expressions. You should validate your expressions loaded from config in your custom providers using `validateThresholds` from `backstage-plugin-scorecard-node`. Invalid annotation expressions are logged and skipped (do not override)
+4. **Error handling**: You should validate your expressions loaded from config in your custom providers using `validateThresholds` from `backstage-plugin-scorecard-node`. Invalid expressions will result in evaluation error.
 
 ### Best Practices
 
 ### 1. Logical Ordering
 
 Order rules from most restrictive to least restrictive.
 
-In this example, the `error` rule would never be triggered because any value greater than 20 would already match the `warning` rule (since it is evaluated first). As a result, all values above 50 would be classified as `warning` instead of `error`:
+In this example, the `success` rule would never be triggered because any value smaller than 15 would already match the `warning` rule (since it is evaluated first). As a result, all values under 15 would be classified as `warning` instead of `success`:
 
 ```yaml
 rules:
   - key: warning
-    expression: '>20'
-  - key: error
-    expression: '>50'
+    expression: '<30'
   - key: success
-    expression: '<=20'
+    expression: '<15'
+  - key: error
+    expression: '>30'
 ```
 
 ### 2. Avoid Overlapping Ranges

workspaces/scorecard/plugins/scorecard-backend/src/plugin.ts

@@ -57,7 +57,6 @@ export const scorecardPlugin = createBackendPlugin({
         auth: coreServices.auth,
         httpRouter: coreServices.httpRouter,
         catalog: catalogServiceRef,
-        logger: coreServices.logger,
         httpAuth: coreServices.httpAuth,
         permissions: coreServices.permissions,
         permissionsRegistry: coreServices.permissionsRegistry,
@@ -66,7 +65,6 @@ export const scorecardPlugin = createBackendPlugin({
         discovery,
         auth,
         httpRouter,
-        logger,
         httpAuth,
         permissions,
         permissionsRegistry,
@@ -82,7 +80,6 @@ export const scorecardPlugin = createBackendPlugin({
           catalogApi: catalogClient,
           registry: metricProvidersRegistry,
           thresholdEvaluator: new ThresholdEvaluator(),
-          logger,
           auth,
         });