Extract BigQuery into DataProvider interface for Component Readiness#3438
Merged
openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom Apr 16, 2026
Merged
Conversation
Introduces a DataProvider abstraction layer that decouples Component Readiness from direct BigQuery access. BigQuery query logic moves from pkg/api/componentreadiness into pkg/api/componentreadiness/dataprovider/bigquery, behind a clean interface defined in dataprovider/interface.go. All callers (server, metrics, jira automator, job annotator, cache loader) now use the DataProvider interface instead of raw BigQuery clients. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Member
Author
|
/test e2e |
Contributor
WalkthroughIntroduces a dataprovider abstraction for Component Readiness, a BigQuery-backed provider implementation, and backend-agnostic crstatus types; rewires callers (server, CLI, metrics, loaders, middleware, regressions, Jira automator, tests) to use the provider instead of direct BigQuery clients and BQ-specific types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DataProvider
participant BigQuery
participant Cache
Client->>Server: HTTP/CLI request (component report / test details / metrics)
Server->>DataProvider: QueryJobVariants / QueryReleases / QueryBaseTestStatus...
DataProvider->>Cache: Get cached results (if present)
alt cache miss
DataProvider->>BigQuery: Execute SQL via query generators
BigQuery-->>DataProvider: Rows / results
DataProvider->>Cache: Store generated results
end
DataProvider-->>Server: Aggregated crstatus types
Server-->>Client: JSON response
Note over Server,DataProvider: Metrics refresh and Jira automator use same flow asynchronously
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmd/sippy/automatejira.go (1)
168-174:⚠️ Potential issue | 🔴 CriticalGuard
configbefore dereferencing overrides.Line 170 treats config load failure as non-fatal, but Line 173 dereferences
config.ComponentReadinessConfig...unconditionally. A missing or unreadable config file now turns that warning into a panic before Jira automation starts.🛠️ Proposed fix
- provider := bqprovider.NewBigQueryProvider(bigQueryClient, config.ComponentReadinessConfig.VariantJunitTableOverrides) + provider := bqprovider.NewBigQueryProvider(bigQueryClient, nil) + if config != nil { + provider = bqprovider.NewBigQueryProvider(bigQueryClient, config.ComponentReadinessConfig.VariantJunitTableOverrides) + } allVariants, errs := componentreadiness.GetJobVariants(ctx, provider)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/sippy/automatejira.go` around lines 168 - 174, The code calls f.ConfigFlags.GetConfig and logs an error but then unconditionally dereferences config.ComponentReadinessConfig when constructing bqprovider.NewBigQueryProvider, which can panic if config is nil; update the logic in the function containing these lines so that after GetConfig fails you either return the error (stop startup) or create/assign a safe default config before using config.ComponentReadinessConfig; specifically guard the config variable before passing config.ComponentReadinessConfig.VariantJunitTableOverrides into bqprovider.NewBigQueryProvider and ensure componentreadiness.GetJobVariants receives a valid provider only when config is non-nil (or the default is applied).pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go (1)
195-200:⚠️ Potential issue | 🟠 MajorTreat empty error slices as success in both release-date lookups.
Both checks use
errs != nil. A provider that returns[]error{}on success will bail out here before any fallback data is loaded.Suggested fix
timeRanges, errs := r.dataProvider.QueryReleaseDates(ctx, r.reqOptions) for _, err := range errs { errCh <- err } - if errs != nil { + if len(errs) > 0 { return }timeRanges, errs := f.dataProvider.QueryReleaseDates(ctx, f.ReqOptions) - if errs != nil { + if len(errs) > 0 { return nil, errs }Also applies to: 362-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go` around lines 195 - 200, The loop over errors from r.dataProvider.QueryReleaseDates uses "errs != nil" to decide failure, which treats empty slices as errors; change the check to test length (len(errs) > 0) so only non-empty error slices cause an early return, and do the same fix for the second lookup block around the code referenced at 362-366; ensure you still send each err into errCh before returning when len(errs) > 0 and keep using the existing symbols (timeRanges, errs, r.dataProvider.QueryReleaseDates, errCh, r.reqOptions) so behavior is preserved for true errors.pkg/sippyserver/server.go (1)
408-416:⚠️ Potential issue | 🟠 MajorDon't advertise full component-readiness capability from
crDataProvideralone.This now enables
ComponentReadinessCapabilityfor provider-only deployments, but handlers likejsonPullRequestTestResults,jsonTestRunsAndOutputsFromBigQuery,jsonJobRunPayload,jsonTestCapabilitiesFromDB, andjsonTestLifecyclesFromDBstill hard-requires.bigQueryClient. Those endpoints will show up in/apiand then fail at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sippyserver/server.go` around lines 408 - 416, The capability detection in determineCapabilities currently adds ComponentReadinessCapability when either s.bigQueryClient or s.crDataProvider is present, which wrongly advertises endpoints that actually require BigQuery; update determineCapabilities to only append ComponentReadinessCapability when s.bigQueryClient != nil (or otherwise ensure s.bigQueryClient is present alongside s.crDataProvider) so handlers like jsonPullRequestTestResults, jsonTestRunsAndOutputsFromBigQuery, jsonJobRunPayload, jsonTestCapabilitiesFromDB and jsonTestLifecyclesFromDB are not advertised unless s.bigQueryClient is available.pkg/api/componentreadiness/component_report.go (1)
337-405:⚠️ Potential issue | 🟠 MajorRemove the unused
baseStatusCh.Line 337 creates
baseStatusChand line 348 passes it intoc.middlewares.Query, but nothing reads from that channel. The TODO comment confirms it is intentionally unused. None of the middleware implementations (LinkInjector, RegressionTracker) send to it—they only useerrCh. The channel is created, passed, and closed, but never consumed. Remove it from the channel creation, the Query call signature, and the cleanup goroutine to eliminate dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/componentreadiness/component_report.go` around lines 337 - 405, Remove the unused baseStatusCh and its related operations: stop creating baseStatusCh, remove it from the c.middlewares.Query call, and stop closing it in the cleanup goroutine (remove close(baseStatusCh)). Search for the symbol baseStatusCh and the call site c.middlewares.Query(...) in component_report.go to update the Query invocation signature accordingly and remove the now-dead channel cleanup that references close(baseStatusCh).
🧹 Nitpick comments (3)
pkg/dataloader/crcacheloader/crcacheloader.go (1)
203-203: Consider usingl.dataProvider.Cache()for consistency.The code uses
l.bqClient.Cachedirectly for cache operations, but theDataProviderinterface provides aCache()method. Usingl.dataProvider.Cache()would be more consistent with the abstraction pattern introduced in this PR.♻️ Suggested change
- api.CacheSet(ctx, l.bqClient.Cache, report, cacheKey, cacheDuration) + api.CacheSet(ctx, l.dataProvider.Cache(), report, cacheKey, cacheDuration)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dataloader/crcacheloader/crcacheloader.go` at line 203, The call to api.CacheSet uses the concrete l.bqClient.Cache instead of the DataProvider abstraction; change the cache argument to use l.dataProvider.Cache() so cache operations go through the DataProvider interface (update the invocation of api.CacheSet(ctx, l.bqClient.Cache, report, cacheKey, cacheDuration) to pass l.dataProvider.Cache() instead), ensuring consistency with the DataProvider abstraction in crcacheloader.go and related methods that expect DataProvider-provided cache.pkg/apis/api/componentreport/crstatus/types.go (1)
10-14: Move package documentation before thepackagedeclaration.The package-level documentation comment (lines 10-13) is placed after the imports, but Go conventions place package documentation immediately before the
packagekeyword so thatgo docand documentation tools render it correctly.📝 Suggested fix
+// Package crstatus contains data-transfer types used between the data layer and +// the Component Readiness analysis pipeline. These types were originally in the +// bq package but are backend-agnostic — any data provider (BigQuery, PostgreSQL, +// mock) populates them identically. package crstatus import ( "math/big" "time" "github.com/openshift/sippy/pkg/apis/api/componentreport/crtest" ) - -// Package crstatus contains data-transfer types used between the data layer and -// the Component Readiness analysis pipeline. These types were originally in the -// bq package but are backend-agnostic — any data provider (BigQuery, PostgreSQL, -// mock) populates them identically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/apis/api/componentreport/crstatus/types.go` around lines 10 - 14, The package-level documentation block for the crstatus types should be moved so it immediately precedes the package declaration; relocate the current comment block (the explanatory lines about "Package crstatus contains data-transfer types...") to sit directly above the `package crstatus` line in types.go so that `go doc` and other tooling pick it up as the package comment.pkg/componentreadiness/jiraautomator/jiraautomator.go (1)
71-105: Remove the constructor's unconditional BigQuery gate.The report path now goes through
provider, but Lines 100-105 still reject any caller without abqClient. That keepsJiraAutomatorunusable with a non-BigQueryDataProvider, which is exactly the abstraction this PR is introducing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/componentreadiness/jiraautomator/jiraautomator.go` around lines 71 - 105, The NewJiraAutomator constructor currently rejects callers with a nil BigQuery client via the checks on bqClient/BQ and bqClient.Cache; remove those unconditional guards (the if blocks referencing bqClient == nil || bqClient.BQ == nil and bqClient.Cache == nil) so JiraAutomator can be constructed when the DataProvider path is used; ensure NewJiraAutomator simply assigns bqClient into the JiraAutomator struct and defer any BigQuery-specific validation to the code paths that actually call bqClient (or add guarded nil checks where bqClient is used) so the provider-based report flow works without requiring a BigQuery client upfront.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/api/componentreadiness/dataprovider/bigquery/provider.go`:
- Around line 615-624: The function shouldSkipVariant currently returns false as
soon as it hits the current override index, which stops scanning later
overrides; change the logic in shouldSkipVariant to skip the comparison for i ==
currOverride (use continue/skip that iteration) instead of returning, so the
loop can still detect and return true if any later override
(override.VariantName == key && override.VariantValue == value) matches; keep
the final return false if no matches are found.
In `@pkg/api/componentreadiness/dataprovider/bigquery/querygenerators.go`:
- Around line 664-676: The loop wrongly reads from
c.VariantOption.IncludeVariants instead of using the passed includeVariants map;
update the body to use includeVariants[key] when building the filter so the
caller's overridden include-variants are respected. Specifically, inside the
loop that iterates sortedKeys(includeVariants) and checks
testIDOption.RequestedVariants and c.VariantOption.VariantCrossCompare, replace
the reference to c.VariantOption.IncludeVariants[key] with includeVariants[key]
(still use param.Cleanse for group and FormatStringSliceForBigQuery to format
the slice) so the constructed query uses the provided map.
- Around line 1088-1091: The code directly type-asserts row[i] for the
"jira_component" and "jira_component_id" cases (setting cts.JiraComponent and
cts.JiraComponentID) which will panic if BigQuery returns NULL; update the
switch handling in the function that iterates columns (the block with cases
"jira_component" and "jira_component_id") to first check for nil (e.g., if
row[i] == nil) and only then perform the type assertion, otherwise leave
cts.JiraComponent empty/zero-value and cts.JiraComponentID nil; use safe type
assertions (value, ok) or nil checks to avoid panics and ensure downstream
optionality is preserved.
- Around line 270-313: The dedup partition currently groups by file_path,
test_name, testsuite which collapses identical tests across different runs;
update the ROW_NUMBER PARTITION BY in the dedupedCTE (variable dedupedCTE) to
include the job run identifier (e.g., junit.prowjob_build_id or
prowjob_build_id) so deduping happens per job run: change PARTITION BY
file_path, test_name, testsuite to PARTITION BY prowjob_build_id, file_path,
test_name, testsuite (keep the ORDER BY logic and rest of the SELECT the same).
In `@pkg/api/componentreadiness/dataprovider/bigquery/releasedates.go`:
- Around line 14-20: The cache key for GetReleaseDatesFromBigQuery is a global
singleton ("CRReleaseDates~") but QueryReleaseDates (on releaseDateQuerier) uses
reqOptions.CacheOption.CRTimeRoundingFactor to compute Start, so different
rounding factors can collide; update the cache spec key created in
GetReleaseDatesFromBigQuery (api.NewCacheSpec call) to include the
CRTimeRoundingFactor (and any other relevant reqOptions.CacheOption fields) so
the cache key is unique per rounding factor, e.g. append or interpolate
reqOptions.CacheOption.CRTimeRoundingFactor into the "CRReleaseDates~" key
generation before calling api.NewCacheSpec.
In `@pkg/api/componentreadiness/test_details.go`:
- Around line 199-202: The error handling currently checks errs != nil after
calling c.dataProvider.QueryReleaseDates in test_details.go; because
QueryReleaseDates returns a []error, update the branch to check len(errs) > 0
(or len(errs) != 0) instead of errs != nil so empty slices don't trigger an
early return; locate the call to QueryReleaseDates and the variable errs and
change the condition before returning testdetails.Report{} to use the length
check.
In `@pkg/sippyserver/metrics/metrics.go`:
- Around line 122-129: The code only fills the releases slice when crProvider !=
nil, causing job/payload/disruption metrics (which use releases) to be empty if
component readiness is disabled; change the logic so releases is always
populated: call crProvider.QueryReleases(ctx) when crProvider is non-nil,
otherwise call a fallback release loader (e.g., your cluster/cached release
fetch function) to populate the same releases slice, and propagate/handle errors
the same way; update the block around the releases variable (the releases slice,
crProvider, and QueryReleases(ctx)) to ensure releases is non-nil before
downstream metric functions use it.
---
Outside diff comments:
In `@cmd/sippy/automatejira.go`:
- Around line 168-174: The code calls f.ConfigFlags.GetConfig and logs an error
but then unconditionally dereferences config.ComponentReadinessConfig when
constructing bqprovider.NewBigQueryProvider, which can panic if config is nil;
update the logic in the function containing these lines so that after GetConfig
fails you either return the error (stop startup) or create/assign a safe default
config before using config.ComponentReadinessConfig; specifically guard the
config variable before passing
config.ComponentReadinessConfig.VariantJunitTableOverrides into
bqprovider.NewBigQueryProvider and ensure componentreadiness.GetJobVariants
receives a valid provider only when config is non-nil (or the default is
applied).
In `@pkg/api/componentreadiness/component_report.go`:
- Around line 337-405: Remove the unused baseStatusCh and its related
operations: stop creating baseStatusCh, remove it from the c.middlewares.Query
call, and stop closing it in the cleanup goroutine (remove close(baseStatusCh)).
Search for the symbol baseStatusCh and the call site c.middlewares.Query(...) in
component_report.go to update the Query invocation signature accordingly and
remove the now-dead channel cleanup that references close(baseStatusCh).
In `@pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go`:
- Around line 195-200: The loop over errors from
r.dataProvider.QueryReleaseDates uses "errs != nil" to decide failure, which
treats empty slices as errors; change the check to test length (len(errs) > 0)
so only non-empty error slices cause an early return, and do the same fix for
the second lookup block around the code referenced at 362-366; ensure you still
send each err into errCh before returning when len(errs) > 0 and keep using the
existing symbols (timeRanges, errs, r.dataProvider.QueryReleaseDates, errCh,
r.reqOptions) so behavior is preserved for true errors.
In `@pkg/sippyserver/server.go`:
- Around line 408-416: The capability detection in determineCapabilities
currently adds ComponentReadinessCapability when either s.bigQueryClient or
s.crDataProvider is present, which wrongly advertises endpoints that actually
require BigQuery; update determineCapabilities to only append
ComponentReadinessCapability when s.bigQueryClient != nil (or otherwise ensure
s.bigQueryClient is present alongside s.crDataProvider) so handlers like
jsonPullRequestTestResults, jsonTestRunsAndOutputsFromBigQuery,
jsonJobRunPayload, jsonTestCapabilitiesFromDB and jsonTestLifecyclesFromDB are
not advertised unless s.bigQueryClient is available.
---
Nitpick comments:
In `@pkg/apis/api/componentreport/crstatus/types.go`:
- Around line 10-14: The package-level documentation block for the crstatus
types should be moved so it immediately precedes the package declaration;
relocate the current comment block (the explanatory lines about "Package
crstatus contains data-transfer types...") to sit directly above the `package
crstatus` line in types.go so that `go doc` and other tooling pick it up as the
package comment.
In `@pkg/componentreadiness/jiraautomator/jiraautomator.go`:
- Around line 71-105: The NewJiraAutomator constructor currently rejects callers
with a nil BigQuery client via the checks on bqClient/BQ and bqClient.Cache;
remove those unconditional guards (the if blocks referencing bqClient == nil ||
bqClient.BQ == nil and bqClient.Cache == nil) so JiraAutomator can be
constructed when the DataProvider path is used; ensure NewJiraAutomator simply
assigns bqClient into the JiraAutomator struct and defer any BigQuery-specific
validation to the code paths that actually call bqClient (or add guarded nil
checks where bqClient is used) so the provider-based report flow works without
requiring a BigQuery client upfront.
In `@pkg/dataloader/crcacheloader/crcacheloader.go`:
- Line 203: The call to api.CacheSet uses the concrete l.bqClient.Cache instead
of the DataProvider abstraction; change the cache argument to use
l.dataProvider.Cache() so cache operations go through the DataProvider interface
(update the invocation of api.CacheSet(ctx, l.bqClient.Cache, report, cacheKey,
cacheDuration) to pass l.dataProvider.Cache() instead), ensuring consistency
with the DataProvider abstraction in crcacheloader.go and related methods that
expect DataProvider-provided cache.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b12e567-2f09-4d8a-a823-7326837b3fbd
📒 Files selected for processing (27)
cmd/sippy/annotatejobruns.gocmd/sippy/automatejira.gocmd/sippy/component_readiness.gocmd/sippy/load.gocmd/sippy/serve.gopkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/querygenerators.gopkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/middleware/interface.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/api/componentreadiness/middleware/list.gopkg/api/componentreadiness/middleware/regressionallowances/regressionallowances.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/regressiontracker.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/utils/utils.gopkg/apis/api/componentreport/crstatus/types.gopkg/apis/api/componentreport/testdetails/types.gopkg/bigquery/bqlabel/labels.gopkg/componentreadiness/jiraautomator/jiraautomator.gopkg/componentreadiness/jobrunannotator/jobrunannotator.gopkg/dataloader/crcacheloader/crcacheloader.gopkg/sippyserver/metrics/metrics.gopkg/sippyserver/server.go
Member
Author
|
/test e2e |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Scheduling required tests: |
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, 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 |
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces a DataProvider abstraction layer that decouples Component Readiness from direct BigQuery access. BigQuery query logic moves from pkg/api/componentreadiness into pkg/api/componentreadiness/dataprovider/bigquery, behind a clean interface defined in dataprovider/interface.go. All callers (server, metrics, jira automator, job annotator, cache loader) now use the DataProvider interface instead of raw BigQuery clients.
The DataProvider interface (pkg/api/componentreadiness/dataprovider/interface.go) is probably not how I would designed it from scratch -- base and sample have difference semantics because of cache optimizations we've made in where results get filtered, but overall I think this is a positive change.
I have proven in another branch that the interface is sufficient to build off of Postgres, but I am extracting these into multiple PR's to reduce the review surface, and impacts once merged.
Summary by CodeRabbit
New Features
Refactor