RHIDP-14172 Refactor average aggregation type for Scorecard card#3417
RHIDP-14172 Refactor average aggregation type for Scorecard card#3417imykhno wants to merge 3 commits into
average aggregation type for Scorecard card#3417Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
|
🤖 Finished Review · ✅ Success · Started 2:19 PM UTC · Completed 2:35 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3417 +/- ##
=======================================
Coverage 54.02% 54.03%
=======================================
Files 2409 2409
Lines 87733 87740 +7
Branches 24306 24308 +2
=======================================
+ Hits 47400 47407 +7
Misses 38812 38812
Partials 1521 1521
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ReviewFindingsCritical
High
Medium
Low
Previous runReviewFindingsHigh
Medium
Low
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| * Supported aggregation types | ||
| * @public | ||
| */ | ||
| export const aggregationTypes = Object.freeze({ |
There was a problem hiding this comment.
[high] exported-constant-rename
The @public exported aggregationTypes constant object loses its average key and gains weightedStatusScore. Any consumer referencing aggregationTypes.average will break at compile time and runtime. The value string also changes from average to weightedStatusScore, breaking any code that persists or compares the string literal.
Suggested fix: Major bump is correctly declared. For backward compatibility during a transition period, consider keeping average as a deprecated alias key on the frozen object so both values are accepted temporarily.
| @@ -101,11 +104,11 @@ export class AverageAggregationStrategy implements AggregationStrategy { | |||
| score: options.statusScores[rule.key] ?? 0, | |||
There was a problem hiding this comment.
[high] api-response-contract
The GET /aggregations/:aggregationId REST API response body changes three field names on the result object (averageScore to weightedStatusScore, averageWeightedSum to weightedStatusSum, averageMaxPossible to weightedStatusMaxPossible) and the metadata.aggregationType string value. Any downstream HTTP client reading these fields will silently get undefined for the old field names.
Suggested fix: Major version bump covers the semver contract. If rolling deployments are possible, consider having the backend serve both old and new field names during a transition window, or document that frontend and backend must be upgraded in lockstep.
| averageMaxPossible: number; | ||
| aggregationChartDisplayColor: string; | ||
| }; | ||
| export type WeightedStatusScoreAggregationResult = |
There was a problem hiding this comment.
[medium] exported-type-removal
The type AggregatedMetricAverageResult is removed and replaced by WeightedStatusScoreAggregationResult. While not annotated with @public JSDoc, it is used in the @public union AggregationResultByType and exported via barrel exports. External consumers importing it will get compile-time errors with no deprecation guidance.
Suggested fix: Consider re-exporting WeightedStatusScoreAggregationResult under the old name as a deprecated alias for at least one release cycle.
| '@red-hat-developer-hub/backstage-plugin-scorecard-backend-module-jira': major | ||
| '@red-hat-developer-hub/backstage-plugin-scorecard-common': major | ||
| '@red-hat-developer-hub/backstage-plugin-scorecard-node': major | ||
| --- |
There was a problem hiding this comment.
[low] scope-creep
The changeset declares major bumps for scorecard-backend-module-github, scorecard-backend-module-jira, and scorecard-node, but the diff contains zero code changes in those three packages and they do not re-export any renamed identifiers.
Suggested fix: Remove those three packages from the changeset unless they re-export renamed types from scorecard-common.
| @@ -119,7 +119,7 @@ describe('AverageAggregationStrategy', () => { | |||
|
|
|||
There was a problem hiding this comment.
[low] naming-coherence
Several test fixtures still use avgKpi as the aggregation config id, referencing the old average concept inconsistently with the otherwise thorough rename.
Suggested fix: Rename test fixture ids from avgKpi to weightedKpi or weightedStatusScoreKpi.
…dStatusScore` Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
… safety and improved structure Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
…dKpi` across tests and configurations Signed-off-by: Ihor Mykhno <imykhno@redhat.com>
imykhno
force-pushed
the
refactor/scorecard-aggregation-type
branch
from
4831a52 to
60f1f11
Compare
|
🤖 Review · Started 8:35 AM UTC |
|
|
🤖 Finished Review · ❌ Failure · Started 8:35 AM UTC · Completed 8:43 AM UTC |
|
/fs-review |
imykhno
requested review from
a team,
Eswaraiahsapram,
christoph-jerolimov and
its-mitesh-kumar
as code owners
|
🤖 Review · Started 9:29 AM UTC |
PR Summary by QodoRefactor Scorecard aggregation type: average → weightedStatusScore Description
Diagram
High-Level Assessment
Files changed (53)
|
Code Review by Qodo
Context used✅ Tickets:
RHIDP-14172 1. Breaking bump level
|
rhdh-qodo-merge
Bot
added
documentation
| averageMaxPossible: number; | ||
| aggregationChartDisplayColor: string; | ||
| }; | ||
| export type WeightedStatusScoreAggregationResult = |
There was a problem hiding this comment.
[critical] breaking-api-change
The public type AggregatedMetricAverageResult (@public in report.api.md) is renamed to WeightedStatusScoreAggregationResult with no deprecated alias. External consumers importing this type get compile-time errors. Fields averageScore, averageWeightedSum, averageMaxPossible are also renamed, breaking runtime consumers.
Suggested fix: Either bump to major, or keep old type as deprecated alias with @deprecated JSDoc tag.
| statusGrouped: STATUS_GROUPED, | ||
| average: AVERAGE, | ||
| weightedStatusScore: WEIGHTED_STATUS_SCORE, | ||
| }); |
There was a problem hiding this comment.
[critical] breaking-api-change
The public constant aggregationTypes.average is renamed to aggregationTypes.weightedStatusScore. JS consumers using aggregationTypes.average get undefined at runtime. Also changes the string literal in API responses and config files.
Suggested fix: Major bump, or add new key while keeping old as deprecated.
| averageMaxPossible: number; | ||
| aggregationChartDisplayColor: string; | ||
| }; | ||
| export type WeightedStatusScoreAggregationResult = |
There was a problem hiding this comment.
[medium] naming-consistency
The old type AggregatedMetricAverageResult followed the AggregatedMetric*Result pattern, while the new name WeightedStatusScoreAggregationResult follows the *AggregationResult pattern used by sibling StatusGroupedAggregationResult. The rename moves toward consistency with the sibling, but the two naming patterns now coexist.
| @@ -101,11 +104,11 @@ export class AverageAggregationStrategy implements AggregationStrategy { | |||
| score: options.statusScores[rule.key] ?? 0, | |||
There was a problem hiding this comment.
[medium] api-response-field-rename
Wire-protocol change affecting REST API consumers beyond TypeScript. HTTP clients parsing JSON fields averageScore, averageWeightedSum, averageMaxPossible by name will break.
|
🤖 Finished Review · ✅ Success · Started 9:29 AM UTC · Completed 9:46 AM UTC |
1 participant
Hey, I just made a Pull Request!
Based on our discussion regarding the new scorecard aggregation types, we decided that the currently implemented
averageis actually a weighted status score — not an average of raw values. Because of this, we have decided to refactor the name of the average type.For:
✔️ Checklist
How to test:
Confirm that
weightedStatusScoreaggregation type works correctly:app-config.yaml, for example: