SREP-4417: Config-driven overview customization for rosa-stage#3465
SREP-4417: Config-driven overview customization for rosa-stage#3465dustman9000 wants to merge 2 commits intoopenshift:mainfrom
Conversation
Adds an `overview` config block to ReleaseConfig so that any release can customize its overview page display via openshift-customizations.yaml. For rosa-stage, this enables: - Multi-version install test queries (both old synthetic and new-style names) - Wider time windows for "New Test Failures" (48h/168h vs 24h/72h) - A "Top Failing Tests" section showing all failures in the past 7 days The overview config flows from the YAML config through the /api/health endpoint to the React frontend, so no release names are hardcoded.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@dustman9000: This pull request references SREP-4403 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
dustman9000
changed the title
|
@dustman9000: This pull request references SREP-4417 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/health.go (1)
78-99: Multi-version install tests logic is correct but consider adding a brief comment.The logic correctly inverts the test name selection based on
useNewInstallTest(release)and picks whichever variant has moreCurrentRuns. This handles releases spanning OCP version boundaries well.Consider adding a brief inline comment explaining why we query both and compare, as the logic may not be immediately obvious to future maintainers.
📝 Optional: Add clarifying comment
// When configured, try both old-style synthetic and new-style install test // names and keep whichever has more data. Useful for releases that span // multiple OCP versions. if overviewCfg != nil && overviewCfg.MultiVersionInstallTests { + // Primary names were set above based on useNewInstallTest(); here we try + // the opposite naming scheme and keep the result with more runs. altInfra := testidentification.NewInfrastructureTestName altInstall := testidentification.NewInstallTestName🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/health.go` around lines 78 - 99, Add a brief inline comment above the MultiVersionInstallTests block explaining that when overviewCfg.MultiVersionInstallTests is enabled we query both old and new test name variants (using testidentification.NewInfrastructureTestName/NewInstallTestName vs InfrastructureTestName/InstallTestName chosen by useNewInstallTest(release)) and then pick the indicator with the higher CurrentRuns (via query.TestReportExcludeVariants) so releases spanning OCP version boundaries use the most-populated metric for indicators["infrastructure"] and indicators["install"]; place the comment immediately before the if overviewCfg... block to make the rationale obvious to future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/api/health.go`:
- Around line 78-99: Add a brief inline comment above the
MultiVersionInstallTests block explaining that when
overviewCfg.MultiVersionInstallTests is enabled we query both old and new test
name variants (using
testidentification.NewInfrastructureTestName/NewInstallTestName vs
InfrastructureTestName/InstallTestName chosen by useNewInstallTest(release)) and
then pick the indicator with the higher CurrentRuns (via
query.TestReportExcludeVariants) so releases spanning OCP version boundaries use
the most-populated metric for indicators["infrastructure"] and
indicators["install"]; place the comment immediately before the if
overviewCfg... block to make the rationale obvious to future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b983e5c5-049f-4e10-ab72-09e52dfc23ef
📒 Files selected for processing (8)
config/openshift-customizations.yamlconfig/openshift.yamlpkg/api/health.gopkg/apis/api/types.gopkg/apis/config/v1/types.gopkg/sippyserver/server.gosippy-ng/src/releases/RecentTestFailures.jssippy-ng/src/releases/ReleaseOverview.js
openshift-merge-bot
Bot
added
the
ready-for-human-review
|
@coderabbitai[bot]: changing LGTM is restricted to collaborators DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: coderabbitai[bot], dustman9000 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
- Fix prettier violation in ReleaseOverview.js (single-line expression) - Add inline comment explaining multi-version install test query logic - Add unit tests for useNewInstallTest covering versioned and non-versioned release names
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/health.go (1)
58-90: Avoid duplicateuseNewInstallTest(release)calls in the same path.Line 58 and Line 87 call the same helper with identical args. Cache once and reuse for both primary and alternate test-name selection.
♻️ Proposed refactor
infraTestName := testidentification.InfrastructureTestName installTestName := testidentification.InstallTestName - if useNewInstallTest(release) { + useNewInstall := useNewInstallTest(release) + if useNewInstall { infraTestName = testidentification.NewInfrastructureTestName installTestName = testidentification.NewInstallTestName } ... if overviewCfg != nil && overviewCfg.MultiVersionInstallTests { altInfra := testidentification.NewInfrastructureTestName altInstall := testidentification.NewInstallTestName - if useNewInstallTest(release) { + if useNewInstall { altInfra = testidentification.InfrastructureTestName altInstall = testidentification.InstallTestName }As per coding guidelines, "Avoid calling the same utility function multiple times with identical arguments in the same code path."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/health.go` around lines 58 - 90, Cache the boolean result of useNewInstallTest(release) in a local variable (e.g., useNew := useNewInstallTest(release)) and replace the duplicate calls: use useNew when deciding infraTestName and installTestName (where infraTestName and installTestName are set) and again when choosing altInfra and altInstall in the overviewCfg.MultiVersionInstallTests branch; this removes the duplicate function calls while preserving the existing logic and identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/api/health.go`:
- Around line 58-90: Cache the boolean result of useNewInstallTest(release) in a
local variable (e.g., useNew := useNewInstallTest(release)) and replace the
duplicate calls: use useNew when deciding infraTestName and installTestName
(where infraTestName and installTestName are set) and again when choosing
altInfra and altInstall in the overviewCfg.MultiVersionInstallTests branch; this
removes the duplicate function calls while preserving the existing logic and
identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b1cb2f00-5f55-4135-936f-f91ae18367f6
📒 Files selected for processing (3)
pkg/api/health.gopkg/api/health_test.gosippy-ng/src/releases/ReleaseOverview.js
|
Scheduling required tests: |
|
@dustman9000: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Jira: https://redhat.atlassian.net/browse/SREP-4417
overviewconfig block toReleaseConfigso any release can customize its overview page viaopenshift-customizations.yaml/api/healthresponse -> React frontend with no hardcoded release namesConfig example
Test plan
/api/health?release=rosa-stageresponse includesoverviewfield/api/health?release=4.17response does not includeoverviewfieldSummary by CodeRabbit
New Features
Configuration