TRT-2501: feat(component readiness): allow filtering by test lifecycle #3201
Conversation
Add Lifecycles filter in sidebar to filter sample results by test lifecycle (blocking/informing). NULL or empty values are treated as blocking. Filter applies to sample only, not basis. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThe changes add lifecycle-based filtering capabilities to the component readiness feature across both backend and frontend layers. Backend additions include a new database query function fetching available lifecycles, a new API endpoint serving this data, parameter parsing for lifecycle filters, and query generation logic applying lifecycle filters. Frontend additions include state management for lifecycle selection, context providers for data sharing, and UI components for filtering. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant FrontEnd as Frontend React
participant API as Backend API
participant DB as BigQuery
Client->>FrontEnd: Load Component Readiness
FrontEnd->>API: GET /api/tests/lifecycles
API->>DB: SELECT DISTINCT lifecycle FROM junit<br/>(last 7 days)
DB-->>API: Return lifecycles list
API-->>FrontEnd: JSON response with lifecycles
FrontEnd->>FrontEnd: Populate TestLifecyclesContext
Client->>FrontEnd: Select lifecycles in filter
FrontEnd->>FrontEnd: Update testLifecycles state
FrontEnd->>FrontEnd: Build query with testLifecycles param
Client->>FrontEnd: Generate/Submit report
FrontEnd->>API: GET /api/... ?testLifecycles=x&testLifecycles=y
API->>DB: SELECT ... WHERE lifecycle IN (x, y)
DB-->>API: Filtered component readiness data
API-->>FrontEnd: JSON response
FrontEnd->>FrontEnd: Render filtered results
FrontEnd-->>Client: Display updated report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@stbenjam: This pull request references TRT-2501 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @sippy-ng/src/component_readiness/SidebarTestFilters.js:
- Around line 24-33: The early return in SidebarTestFilters.js currently happens
before the hooks, making the useContext calls for varsContext, testCapabilities
and testLifecycles conditional and violating the Rules of Hooks; move the
useContext calls (for CompReadyVarsContext, TestCapabilitiesContext,
TestLifecyclesContext) to execute unconditionally at the top of the component,
then perform the conditional check on props.controlsOpts?.filterByCapabilities
and props.controlsOpts?.filterByLifecycles and return <Fragment /> afterwards so
hooks are always called in the same order.
🧹 Nitpick comments (3)
pkg/api/tests.go (2)
571-575: Consider handlingiterator.Donefor empty result sets.The current error handling wraps all errors from
it.Next(), but if the query returns no rows (empty table or all NULL/empty lifecycles),iterator.Donewould be returned and wrapped as an error. The existingGetTestCapabilitiesFromDBhas the same pattern, so this is consistent, but worth noting that an empty result might surface as an error rather than an empty slice.Optional: Handle iterator.Done explicitly
err = it.Next(&row) - if err != nil { + if err == iterator.Done { + return []string{}, nil + } else if err != nil { log.WithError(err).Error("error retrieving test lifecycles from bigquery") return []string{}, errors.Wrap(err, "error retrieving test lifecycles from bigquery") }
554-558: Minor: Consider using backticks for table reference consistency.The
GetTestCapabilitiesFromDBfunction uses backticks around the table reference (`%s.component_mapping_latest`), while this query omits them for%s.junit. Both work, but backticks are the standard BigQuery convention for fully-qualified table names.Optional: Add backticks for consistency
qFmt := `SELECT ARRAY_AGG(DISTINCT lifecycle ORDER BY lifecycle) AS lifecycles - FROM %s.junit + FROM `+"`%s.junit`"+` WHERE modified_time >= DATETIME_SUB(CURRENT_DATETIME(), INTERVAL 7 DAY) AND lifecycle IS NOT NULL AND lifecycle != ''`sippy-ng/src/component_readiness/SidebarTestFilters.js (1)
34-49:makeStylescalled inside component body on every render.While not introduced by this PR,
makeStylesshould be called outside the component to avoid recreating styles on each render.♻️ Optional: Move useStyles outside component
+const useStyles = makeStyles((theme) => ({ + formControl: { + margin: theme.spacing(1), + }, + autocomplete: { + width: '100%', + '& .MuiAutocomplete-inputRoot': { + paddingRight: '9px !important', + }, + }, + chip: { + maxWidth: 'none', + flex: '1 1 auto', + }, +})) + export default function SidebarTestFilters(props) { // ... hooks ... - const useStyles = makeStyles((theme) => ({ - formControl: { - margin: theme.spacing(1), - }, - // ... - })) - const classes = useStyles()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
pkg/api/componentreadiness/query/querygenerators.gopkg/api/componentreadiness/utils/queryparamparser.gopkg/api/tests.gopkg/apis/api/componentreport/reqopts/types.gopkg/sippyserver/server.gosippy-ng/src/component_readiness/CompReadyEnvCapabilities.jssippy-ng/src/component_readiness/CompReadyUtils.jssippy-ng/src/component_readiness/CompReadyVars.jssippy-ng/src/component_readiness/ComponentReadiness.jssippy-ng/src/component_readiness/SidebarTestFilters.js
🧰 Additional context used
🧬 Code graph analysis (7)
sippy-ng/src/component_readiness/CompReadyEnvCapabilities.js (1)
sippy-ng/src/component_readiness/Sidebar.js (2)
Sidebar(14-61)theme(16-16)
pkg/api/componentreadiness/utils/queryparamparser.go (1)
pkg/apis/api/componentreport/reqopts/types.go (1)
TestFilters(60-63)
pkg/sippyserver/server.go (2)
pkg/api/tests.go (1)
GetTestLifecyclesFromDB(548-578)pkg/sippyserver/capabilities.go (1)
ComponentReadinessCapability(15-15)
sippy-ng/src/component_readiness/SidebarTestFilters.js (2)
sippy-ng/src/component_readiness/ComponentReadiness.js (4)
testCapabilities(421-421)TestCapabilitiesContext(185-185)TestCapabilitiesContext(185-185)testLifecycles(438-438)sippy-ng/src/component_readiness/CompReadyVars.js (2)
testCapabilities(227-227)testLifecycles(228-228)
sippy-ng/src/component_readiness/ComponentReadiness.js (1)
sippy-ng/src/component_readiness/SidebarTestFilters.js (1)
testLifecycles(33-33)
pkg/api/componentreadiness/query/querygenerators.go (1)
vendor/github.com/yosida95/uritemplate/v3/value.go (1)
Value(50-53)
sippy-ng/src/component_readiness/CompReadyVars.js (4)
sippy-ng/src/component_readiness/ComponentReadiness.js (1)
testLifecycles(438-438)sippy-ng/src/component_readiness/SidebarTestFilters.js (1)
testLifecycles(33-33)sippy-ng/src/component_readiness/CompReadyUtils.js (1)
params(468-468)sippy-ng/src/component_readiness/TestDetailsReport.js (1)
params(323-323)
🪛 Biome (2.1.2)
sippy-ng/src/component_readiness/SidebarTestFilters.js
[error] 31-31: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 32-32: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 33-33: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
sippy-ng/src/component_readiness/ComponentReadiness.js
[error] 438-438: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 458-458: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (18)
sippy-ng/src/component_readiness/CompReadyUtils.js (1)
444-446: LGTM!The implementation correctly mirrors the existing
testCapabilitiespattern at lines 441-443, maintaining consistency in how array parameters are appended to the query string.sippy-ng/src/component_readiness/CompReadyEnvCapabilities.js (1)
206-209: LGTM!The addition of
filterByLifecycles: truetocontrolsOptscorrectly enables the lifecycle filter in the sidebar for the environment capabilities view, following the existing pattern forfilterByCapabilities.pkg/sippyserver/server.go (2)
898-907: LGTM!The handler implementation correctly mirrors the existing
jsonTestCapabilitiesFromDBpattern with proper error handling and logging.
2164-2170: LGTM!The endpoint registration is consistent with the
/api/tests/capabilitiesendpoint - using the same capability requirement (ComponentReadinessCapability) and cache duration (1 hour).pkg/apis/api/componentreport/reqopts/types.go (1)
62-62: LGTM!The
Lifecyclesfield addition follows the same pattern as the existingCapabilitiesfield, with appropriateomitemptytags ensuring backward compatibility.pkg/api/componentreadiness/query/querygenerators.go (1)
377-385: LGTM! Lifecycle filter implementation follows established patterns.The implementation correctly:
- Applies the filter only to sample queries (
isSample && len(reqOptions.Lifecycles) > 0)- Uses parameterized queries to prevent SQL injection
- Handles NULL/empty lifecycle values as "blocking" via
COALESCE(NULLIF(lifecycle, ''), 'blocking')This mirrors the existing
Capabilitiesfilter pattern on lines 366-376.pkg/api/componentreadiness/utils/queryparamparser.go (2)
75-76: LGTM! Query parameter parsing mirrors existing capabilities pattern.The
testLifecyclesparameter parsing follows the same pattern astestCapabilitieson line 75, maintaining consistency.
163-163: Correctly markstestLifecyclesas incompatible with views.This ensures users cannot combine lifecycle filters with predefined views, consistent with the treatment of
testCapabilities.sippy-ng/src/component_readiness/ComponentReadiness.js (4)
186-186: LGTM! Context creation follows existing pattern.The
TestLifecyclesContextis created identically toTestCapabilitiesContexton line 185, maintaining consistency.
438-456: Good error handling infetchLifecycles.The implementation correctly:
- Doesn't fail the entire page on lifecycle fetch errors (logs and sets empty array)
- Ensures state is always an array via
Array.isArray()check- Follows the same pattern as
fetchCapabilitiesNote: The static analysis warning about conditional hooks is a pre-existing issue in this component (early returns at lines 290 and 318 occur before hooks). This PR doesn't introduce that pattern.
490-761: Provider properly wraps component tree.The
TestLifecyclesContext.Providercorrectly wraps the Routes subtree, making lifecycle data available to all component readiness pages.
600-603: Correctly enables lifecycle filtering on main route.The
filterByLifecycles: trueoption enables the new Lifecycles filter in the Sidebar component for the main component readiness view.sippy-ng/src/component_readiness/SidebarTestFilters.js (1)
125-171: LGTM! Lifecycles Autocomplete mirrors Capabilities implementation.The new Lifecycles filter correctly:
- Uses defensive
Array.isArray()checks for both options and value- Follows the same UI pattern as the Capabilities filter
- Adds appropriate margin-top spacing to separate from Capabilities
sippy-ng/src/component_readiness/CompReadyVars.js (5)
110-110: LGTM! Query parameter definition follows existing pattern.
testLifecyclesis defined asArrayParam, consistent withtestCapabilitieson line 109.
227-228: LGTM! State management mirrorstestCapabilities.The
testLifecyclesstate variable and setter follow the same pattern astestCapabilitieson line 227.
326-327: LGTM! State initialization from URL params is consistent.Initialization follows the same pattern as
testCapabilitieson line 326, defaulting to an empty array.
367-368: LGTM! URL propagation in report generation is correct.The
testLifecyclesvalue is included insetParamsalongsidetestCapabilities, ensuring it persists in the URL when generating reports.
618-619: LGTM! Context exposure is complete.Both
testLifecyclesandsetTestLifecyclesare exposed in the provider, enabling consumers likeSidebarTestFiltersto read and update the lifecycle filter state.
| if ( | ||
| !props.controlsOpts?.filterByCapabilities && | ||
| !props.controlsOpts?.filterByLifecycles | ||
| ) { | ||
| // if we have no filters to show, omit the whole component | ||
| return <Fragment /> | ||
| } | ||
| const varsContext = useContext(CompReadyVarsContext) | ||
| const testCapabilities = useContext(TestCapabilitiesContext) | ||
| const testLifecycles = useContext(TestLifecyclesContext) |
There was a problem hiding this comment.
Hooks called after early return violates Rules of Hooks.
The early return on lines 24-30 causes useContext calls on lines 31-33 to be conditional. React requires hooks to be called in the same order on every render.
🐛 Proposed fix: Move early return after hooks
export default function SidebarTestFilters(props) {
+ const varsContext = useContext(CompReadyVarsContext)
+ const testCapabilities = useContext(TestCapabilitiesContext)
+ const testLifecycles = useContext(TestLifecyclesContext)
+
if (
!props.controlsOpts?.filterByCapabilities &&
!props.controlsOpts?.filterByLifecycles
) {
// if we have no filters to show, omit the whole component
return <Fragment />
}
- const varsContext = useContext(CompReadyVarsContext)
- const testCapabilities = useContext(TestCapabilitiesContext)
- const testLifecycles = useContext(TestLifecyclesContext)
const useStyles = makeStyles((theme) => ({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| !props.controlsOpts?.filterByCapabilities && | |
| !props.controlsOpts?.filterByLifecycles | |
| ) { | |
| // if we have no filters to show, omit the whole component | |
| return <Fragment /> | |
| } | |
| const varsContext = useContext(CompReadyVarsContext) | |
| const testCapabilities = useContext(TestCapabilitiesContext) | |
| const testLifecycles = useContext(TestLifecyclesContext) | |
| const varsContext = useContext(CompReadyVarsContext) | |
| const testCapabilities = useContext(TestCapabilitiesContext) | |
| const testLifecycles = useContext(TestLifecyclesContext) | |
| if ( | |
| !props.controlsOpts?.filterByCapabilities && | |
| !props.controlsOpts?.filterByLifecycles | |
| ) { | |
| // if we have no filters to show, omit the whole component | |
| return <Fragment /> | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 31-31: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 32-32: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 33-33: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In @sippy-ng/src/component_readiness/SidebarTestFilters.js around lines 24 - 33,
The early return in SidebarTestFilters.js currently happens before the hooks,
making the useContext calls for varsContext, testCapabilities and testLifecycles
conditional and violating the Rules of Hooks; move the useContext calls (for
CompReadyVarsContext, TestCapabilitiesContext, TestLifecyclesContext) to execute
unconditionally at the top of the component, then perform the conditional check
on props.controlsOpts?.filterByCapabilities and
props.controlsOpts?.filterByLifecycles and return <Fragment /> afterwards so
hooks are always called in the same order.
|
Scheduling required tests: |
|
Suggest making this live in 4.22 main view |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smg247, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@stbenjam: 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. |
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.