Port feature gate promotion logic from o/api to sippy - #3871
Conversation
Query prow jobs with a matching Capability variant for the feature gate and return them in the detail API response. Display the job names as chips in the top panel of the feature gate detail page. Co-authored-by: Cursor <cursoragent@cursor.com>
Omits failing gate tests for non-default gates other than our own.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin 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 |
openshift-ci
Bot
added
the
approved
dgoodwin
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThe change adds feature-gate promotion evaluation across the API and UI. It adds variant and regression analysis, a feature-gate detail endpoint, promotion-focused links and tabs, seeded scenarios, documentation, and end-to-end coverage. ChangesFeature-gate promotion
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FeatureGateDetail
participant FeatureGateDetailHandler
participant GetPromotionStatus
participant QueryTestResults
participant Database
FeatureGateDetail->>FeatureGateDetailHandler: request feature-gate detail
FeatureGateDetailHandler->>Database: load feature gate and matching jobs
FeatureGateDetailHandler->>GetPromotionStatus: compute promotion status
GetPromotionStatus->>QueryTestResults: query gate tests and regressions
QueryTestResults->>Database: execute filtered test-results query
QueryTestResults-->>GetPromotionStatus: return test results
GetPromotionStatus-->>FeatureGateDetailHandler: return promotion status
FeatureGateDetailHandler-->>FeatureGateDetail: return detail and promotion data
🚥 Pre-merge checks | ✅ 16 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sippy-ng/src/tests/FeatureGateDetail.jsx (1)
42-67: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIgnore a stale response when
featureGatechanges.The effect has no cancellation. If the user switches gates quickly, the first response can resolve after the second and
setGatewrites the older gate into state. The header then shows the new gate name while the body shows the previous gate's data. Track the current request and discard outdated results.🛡️ Proposed fix
useEffect(() => { + const controller = new AbortController() document.title = `Sippy > ${release} > Feature Gates > ${featureGate}` setLoaded(false) setFetchError('') fetch( import.meta.env.VITE_API_URL + `/api/feature_gates/${encodeURIComponent( featureGate - )}?release=${encodeURIComponent(release)}` + )}?release=${encodeURIComponent(release)}`, + { signal: controller.signal } ) .then((response) => { if (response.status !== 200) { throw new Error('server returned ' + response.status) } return response.json() }) .then((json) => { setGate(json) setLoaded(true) }) .catch((error) => { + if (error.name === 'AbortError') { + return + } setFetchError('Could not retrieve feature gate: ' + error) setLoaded(true) }) + return () => controller.abort() }, [release, featureGate])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sippy-ng/src/tests/FeatureGateDetail.jsx` around lines 42 - 67, Update the useEffect request flow to track whether its fetch is still current, and ignore both successful and failed results from an earlier featureGate or release after dependencies change. Ensure cleanup invalidates the prior request before the next effect runs, preventing stale setGate, setFetchError, and setLoaded updates.
🧹 Nitpick comments (13)
pkg/api/featuregatepromotion/types.go (2)
47-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
JobTiersstring format.
JobTiersholds a comma-separated tier list, asValidateJobTiersand the tests show. Add a short comment so callers do not pass a single tier only.♻️ Proposed comment
type JobVariant struct { Cloud string Architecture string Topology string NetworkStack string OS string - JobTiers string + // JobTiers is a comma-separated list of job tiers to query, e.g. "standard,candidate". + JobTiers string Optional bool }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/featuregatepromotion/types.go` around lines 47 - 55, Add a concise Go doc comment to the JobVariant.JobTiers field documenting that it contains a comma-separated list of tiers, consistent with ValidateJobTiers and its tests.
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisspelled field
CapabilityTestRegessionsin both promotion structs and the converter. The name misses anrin "Regressions". The declaration was copied into the mirrored API type and into the conversion code, so the typo now appears in three places. The JSON tags are spelled correctly, so renaming the Go field does not change the response body.
pkg/api/featuregatepromotion/types.go#L9-L9: rename thePromotionStatusfield toCapabilityTestRegressions.pkg/apis/api/types.go#L1045-L1045: rename theFeatureGatePromotionfield toCapabilityTestRegressions.pkg/sippyserver/server.go#L821-L822: update the range expression and the append target inconvertPromotionStatusto the new field name.Also update the readers in
test/e2e/feature_gate_promotion_test.go, which accessfg.Promotion.CapabilityTestRegessions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/featuregatepromotion/types.go` at line 9, Rename CapabilityTestRegessions to CapabilityTestRegressions in PromotionStatus and FeatureGatePromotion, update the range expression and append target in convertPromotionStatus, and update all readers in test/e2e/feature_gate_promotion_test.go; leave the existing JSON tag unchanged. Affected sites: pkg/api/featuregatepromotion/types.go:9, pkg/apis/api/types.go:1045, and pkg/sippyserver/server.go:821-822, with corresponding test-reader updates in test/e2e/feature_gate_promotion_test.go.pkg/sippyserver/server.go (1)
799-802: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the install-gate check into the promotion package.
strings.Contains(fg.FeatureGate, "Install")repeats the same rule thatGetPromotionStatusapplies inpkg/api/featuregatepromotion/promotion.go(line 79). The two sites must stay in agreement, because theinstall_testslink must be present exactly when the promotion query includes install tests. Export one predicate, for examplefeaturegatepromotion.IsInstallGate(featureGate string) bool, and call it from both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/sippyserver/server.go` around lines 799 - 802, The install-gate detection rule is duplicated between the server link-building logic and GetPromotionStatus. Add and export a shared predicate such as IsInstallGate in the featuregatepromotion package, update GetPromotionStatus and the install_tests block in the server to use it, and remove the direct strings.Contains check so both behaviors remain synchronized.pkg/api/featuregatepromotion/promotion_test.go (1)
316-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hand-rolled
map[string]boolsets withsets.Both tests build membership checks manually. The file already imports
k8s.io/apimachinery/pkg/util/sets. Use it for the tier lookups.♻️ Proposed refactor
func TestDefaultJobTiersIncludeCandidate(t *testing.T) { tiers := JobTiersForVariant(JobVariant{Cloud: "vsphere", Architecture: "amd64", Topology: "ha"}) - tierSet := make(map[string]bool) - for _, tier := range tiers { - tierSet[tier] = true - } + tierSet := sets.New(tiers...) expected := []string{"standard", "informing", "blocking", "candidate"} for _, tier := range expected { - if !tierSet[tier] { + if !tierSet.Has(tier) { t.Errorf("default tiers missing %q, got: %v", tier, tiers) } } } func TestAllRequiredVariantsQueryCandidateTier(t *testing.T) { allVariants := append(append([]JobVariant{}, RequiredSelfManagedJobVariants...), RequiredHypershiftJobVariants...) for _, variant := range allVariants { tiers := JobTiersForVariant(variant) - hasCandidateTier := false - for _, tier := range tiers { - if tier == "candidate" { - hasCandidateTier = true - break - } - } - if !hasCandidateTier { + if !sets.New(tiers...).Has("candidate") { t.Errorf("variant %+v does not query candidate tier", variant) } } }As per coding guidelines: "Use
k8s.io/apimachinery/pkg/util/setsfor deduplicating or collecting unique strings; do not usemap[string]boolas a hand-rolled set."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/featuregatepromotion/promotion_test.go` around lines 316 - 344, Replace the hand-rolled tier membership checks in the affected tests, including TestAllRequiredVariantsQueryCandidateTier, with k8s.io/apimachinery/pkg/util/sets. Build a string set from each tiers result and use the set’s membership API for candidate and expected-tier checks, removing the manual map and boolean loops.Source: Coding guidelines
pkg/apis/api/types.go (1)
1042-1078: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider removing the duplicated promotion structs.
FeatureGatePromotion,FeatureGateVariantResult,FeatureGateTestResult, andFeatureGateCapabilityTestRegressionmirrorPromotionStatus,VariantResult,TestResult, andCapabilityTestRegressioninpkg/api/featuregatepromotion/types.gofield for field, including the JSON tags. The duplication forces the manualconvertPromotionStatusmapping inpkg/sippyserver/server.go(lines 812-852). Every future field addition must be applied in three places.If the intent is to keep
pkg/apis/apifree of a dependency onpkg/api/featuregatepromotion, keep the duplication and state that reason in a comment. Otherwise, embed or alias the promotion types and delete the converter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/api/types.go` around lines 1042 - 1078, The promotion response structs duplicate the canonical types and require manual conversion. Replace FeatureGatePromotion, FeatureGateVariantResult, FeatureGateTestResult, and FeatureGateCapabilityTestRegression with aliases or embedded canonical promotion types from featuregatepromotion, then remove the corresponding convertPromotionStatus mapping; if the package dependency must remain avoided, retain the structs and document that constraint.pkg/db/query/feature_gates.go (1)
100-112: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
Pluckfor this single-column query.Scan(&names)supports this result, butPluck("name", &names)states the intended scalar-slice mapping and matches existing usage. Keepsort.Strings(names)because database collation can differ from Go sorting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/db/query/feature_gates.go` around lines 100 - 112, Update GetMatchingJobsForCapability to use Pluck("name", &names) instead of Scan(&names) for the single-column name query. Preserve the existing error handling and sort.Strings(names) call.pkg/api/featuregatepromotion/promotion.go (3)
178-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the date-window arithmetic.
The code adds one day to today, subtracts eight days for the start, calls
ResolveDateRanges, then subtracts one day from both resolved bounds. The net window is not obvious, and a later reader cannot tell which off-by-one is intentional. Add a short comment that states why the range is shifted before and after resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/featuregatepromotion/promotion.go` around lines 178 - 184, Add a concise explanatory comment adjacent to the tomorrow/start/end calculations around ResolveDateRanges, documenting the intentional pre-resolution one-day shift and post-resolution subtraction, including the resulting date-window semantics and off-by-one handling.
63-99: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPass
ctxinto the database calls.
GetPromotionStatusreceivesctxand forwards it tosippyapi.QueryTestResults, butgetGateTopologies,getPromotedGateNames, andgetCapabilityRegressionsrun gorm queries without it. If the client cancels the HTTP request, those queries continue. These are local gorm calls in this file, so no shared query-layer signature changes are needed.♻️ Proposed change
-func getGateTopologies(dbc *db.DB, release, featureGate string) (sets.Set[string], error) { +func getGateTopologies(ctx context.Context, dbc *db.DB, release, featureGate string) (sets.Set[string], error) { var topologies []string - tx := dbc.DB.Table("feature_gates"). + tx := dbc.DB.WithContext(ctx).Table("feature_gates").Apply the same change to
getPromotedGateNamesandgetCapabilityRegressions, and update the call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/featuregatepromotion/promotion.go` around lines 63 - 99, Thread ctx through the local database helpers used by GetPromotionStatus: update getGateTopologies, getPromotedGateNames, and getCapabilityRegressions to accept the request context and execute their gorm queries with it, then pass ctx from GetPromotionStatus and between the helper calls. Do not change shared query-layer signatures.
304-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCentralize the default job tiers.
The default tier list is duplicated in
JobTiersForVariant; extract it into one package-level value and reuse it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/featuregatepromotion/promotion.go` around lines 304 - 341, Define one package-level value containing the default job tiers, then update JobTiersForVariant to reuse it for both empty and whitespace-only JobTiers inputs instead of duplicating the literal list. Keep the existing ordering and returned behavior unchanged.sippy-ng/src/tests/FeatureGatePromotionTab.jsx (2)
145-149: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftThe thresholds duplicate the Go constants.
requiredTests,requiredRuns, andrequiredPassRaterestateRequiredNumberOfTests,RequiredNumberOfTestRunsPerVariant, andRequiredPassRateOfTestsPerVariantfrompkg/api/featuregatepromotion/promotion.golines 23-25. A change on the Go side leaves this component reporting the old numbers whiledata.sufficientreflects the new ones. The API already returnserrorsandwarningsbuilt from the real constants. Either renderdata.errorsdirectly, or add the thresholds to the promotion payload and read them here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sippy-ng/src/tests/FeatureGatePromotionTab.jsx` around lines 145 - 149, Remove the hardcoded thresholds object from the FeatureGatePromotionTab logic and use the promotion API’s authoritative validation output instead, preferably rendering data.errors and data.warnings directly. Ensure sufficient-state messaging stays consistent with the Go constants RequiredNumberOfTests, RequiredNumberOfTestRunsPerVariant, and RequiredPassRateOfTestsPerVariant.
22-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the theme palette instead of hardcoded pass and fail colors.
These four hex values are applied as cell backgrounds and text colors at lines 573-574, 689-690, and 726-727. They do not adapt to the dark theme, so the light green and light red backgrounds keep dark text on a dark surface and the contrast drops.
sippy-ng/src/tests/TestTable.jsxreadstheme.palette.success.darkandtheme.palette.error.darkfor the same purpose. UseuseTheme()and the palette here for consistency with the Material-UI standards followed elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sippy-ng/src/tests/FeatureGatePromotionTab.jsx` around lines 22 - 25, Replace the hardcoded PASS_COLOR, PASS_TEXT, FAIL_COLOR, and FAIL_TEXT constants in FeatureGatePromotionTab with values derived from Material-UI’s useTheme() palette, matching TestTable’s use of theme.palette.success.dark and theme.palette.error.dark for the corresponding cell backgrounds and text colors. Update the affected component flow so these theme-derived colors are used at all existing pass/fail cell styling sites.Source: Coding guidelines
sippy-ng/src/tests/FeatureGateDetail.jsx (1)
259-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the "Analyze All" path builder into a shared helper.
sippy-ng/src/tests/FeatureGatePromotionTab.jsxlines 167-180 build the same/jobs/{release}/analysis?filters=...URL frommatchingJobswith identical item shape andlinkOperator: 'or'. Move the builder into one module and import it in both components, so a change to the filter shape stays consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sippy-ng/src/tests/FeatureGateDetail.jsx` around lines 259 - 277, Extract the duplicated `/jobs/{release}/analysis?filters=...` URL construction into a shared helper, preserving the existing matching-job filter item shape and `linkOperator: 'or'`. Update the “Analyze All” button in `FeatureGateDetail` and the corresponding flow in `FeatureGatePromotionTab` to import and use this helper.pkg/api/featuregatepromotion/filters.go (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the 92% threshold into a named constant.
The value
92also appears inpkg/api/featuregatepromotion/promotion.go(the raw SQLHAVINGclause and the error message) and in the frontend filter. Define one exported constant in this package and reference it from the SQL and the message, so the filter and the query cannot drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/featuregatepromotion/filters.go` around lines 47 - 48, Define one exported package-level constant for the 92% promotion threshold, then replace the hardcoded value in the filter configuration, the raw SQL HAVING clause, and the related error message with that constant, using appropriate formatting where interpolation is required. Keep the existing threshold behavior unchanged and ensure the frontend filter references the same shared value if it is generated from this package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/featuregatepromotion/filters.go`:
- Line 31: Update the variants filter to use filter.OperatorHasEntry instead of
filter.OperatorContains, matching the exact capability array element and
aligning with CapabilityRegressionsFilter.
In `@pkg/api/featuregatepromotion/promotion.go`:
- Around line 425-436: Update the aggregation around the loop building each
TestResult so the lookback window is chosen once per test name rather than per
row. Accumulate current and previous run, success, failure, and flake counts
separately for all rows, then select the 7-day totals when aggregate current
runs meet RequiredNumberOfTestRunsPerVariant; otherwise use the combined
current-plus-previous totals. Ensure the resulting TestResult totals and
subsequent threshold check use only the selected window.
- Around line 211-218: Update the raw promotion regression query near the
existing never-stable exclusion to also exclude jobs whose variant combinations
contain the aggregated variant, matching CapabilityRegressionsFilter in
filters.go. Preserve the existing never-stable filtering and ensure the query,
failingCount calculation, and gate_job_tests results use the same exclusion
criteria.
- Around line 42-51: Update every JobVariant entry in
OptionalSelfManagedPlatformVariants to set Optional: true explicitly, preserving
all existing cloud, architecture, topology, network stack, and job tier values.
In `@pkg/api/README.md`:
- Around line 598-603: Update featuregatepromotion.getCapabilityRegressions and
its promotion evaluation path to reuse CapabilityRegressionsFilter, including
the aggregated exclusion, so gate_job_tests evaluates the same canonical data as
its HATEOAS link; then retain the README’s “always stay in sync” statement.
In `@pkg/api/tests.go`:
- Around line 356-359: Update the documentation comment for QueryTestResults to
explicitly state that it queries PostgreSQL only through
buildTestsResultsFromPostgres and does not support BigQuery-backed results, so
callers understand its data-source limitation.
In `@sippy-ng/src/tests/FeatureGateDetail.jsx`:
- Around line 119-123: Update the hand-written filters in FeatureGateDetail: at
sippy-ng/src/tests/FeatureGateDetail.jsx lines 119-123, change the current_runs
value from '0' to '1'; at lines 69-77, add the not: true / has entry conditions
for never-stable and aggregated and set linkOperator to 'and', matching the
canonical GateTestFilter definitions.
In `@sippy-ng/src/tests/FeatureGatePromotionTab.jsx`:
- Around line 156-165: Update the minPassRate calculation in the
requiredVariants scan to consider a test result’s pass_percent only when its
total_runs is greater than zero, matching buildVariantResult behavior. Apply the
same condition in the corresponding scan around the secondary occurrence, while
preserving minRuns calculation and the existing zero fallback when no eligible
results exist.
In `@test/e2e/feature_gate_promotion_test.go`:
- Around line 28-32: Ensure the promotion test validates that
fg.Promotion.ResultsByVariant is non-empty before iterating over it. Add
require.NotEmpty immediately before the existing loop, preserving the
per-variant dimension assertions.
In `@test/e2e/feature_gates_test.go`:
- Around line 104-108: Update TestFeatureGateDetailNotFound to assert that the
SippyGet error contains “code 404” instead of only checking that an error
occurred, ensuring the missing feature gate returns the expected HTTP status.
---
Outside diff comments:
In `@sippy-ng/src/tests/FeatureGateDetail.jsx`:
- Around line 42-67: Update the useEffect request flow to track whether its
fetch is still current, and ignore both successful and failed results from an
earlier featureGate or release after dependencies change. Ensure cleanup
invalidates the prior request before the next effect runs, preventing stale
setGate, setFetchError, and setLoaded updates.
---
Nitpick comments:
In `@pkg/api/featuregatepromotion/filters.go`:
- Around line 47-48: Define one exported package-level constant for the 92%
promotion threshold, then replace the hardcoded value in the filter
configuration, the raw SQL HAVING clause, and the related error message with
that constant, using appropriate formatting where interpolation is required.
Keep the existing threshold behavior unchanged and ensure the frontend filter
references the same shared value if it is generated from this package.
In `@pkg/api/featuregatepromotion/promotion_test.go`:
- Around line 316-344: Replace the hand-rolled tier membership checks in the
affected tests, including TestAllRequiredVariantsQueryCandidateTier, with
k8s.io/apimachinery/pkg/util/sets. Build a string set from each tiers result and
use the set’s membership API for candidate and expected-tier checks, removing
the manual map and boolean loops.
In `@pkg/api/featuregatepromotion/promotion.go`:
- Around line 178-184: Add a concise explanatory comment adjacent to the
tomorrow/start/end calculations around ResolveDateRanges, documenting the
intentional pre-resolution one-day shift and post-resolution subtraction,
including the resulting date-window semantics and off-by-one handling.
- Around line 63-99: Thread ctx through the local database helpers used by
GetPromotionStatus: update getGateTopologies, getPromotedGateNames, and
getCapabilityRegressions to accept the request context and execute their gorm
queries with it, then pass ctx from GetPromotionStatus and between the helper
calls. Do not change shared query-layer signatures.
- Around line 304-341: Define one package-level value containing the default job
tiers, then update JobTiersForVariant to reuse it for both empty and
whitespace-only JobTiers inputs instead of duplicating the literal list. Keep
the existing ordering and returned behavior unchanged.
In `@pkg/api/featuregatepromotion/types.go`:
- Around line 47-55: Add a concise Go doc comment to the JobVariant.JobTiers
field documenting that it contains a comma-separated list of tiers, consistent
with ValidateJobTiers and its tests.
- Line 9: Rename CapabilityTestRegessions to CapabilityTestRegressions in
PromotionStatus and FeatureGatePromotion, update the range expression and append
target in convertPromotionStatus, and update all readers in
test/e2e/feature_gate_promotion_test.go; leave the existing JSON tag unchanged.
Affected sites: pkg/api/featuregatepromotion/types.go:9,
pkg/apis/api/types.go:1045, and pkg/sippyserver/server.go:821-822, with
corresponding test-reader updates in test/e2e/feature_gate_promotion_test.go.
In `@pkg/apis/api/types.go`:
- Around line 1042-1078: The promotion response structs duplicate the canonical
types and require manual conversion. Replace FeatureGatePromotion,
FeatureGateVariantResult, FeatureGateTestResult, and
FeatureGateCapabilityTestRegression with aliases or embedded canonical promotion
types from featuregatepromotion, then remove the corresponding
convertPromotionStatus mapping; if the package dependency must remain avoided,
retain the structs and document that constraint.
In `@pkg/db/query/feature_gates.go`:
- Around line 100-112: Update GetMatchingJobsForCapability to use Pluck("name",
&names) instead of Scan(&names) for the single-column name query. Preserve the
existing error handling and sort.Strings(names) call.
In `@pkg/sippyserver/server.go`:
- Around line 799-802: The install-gate detection rule is duplicated between the
server link-building logic and GetPromotionStatus. Add and export a shared
predicate such as IsInstallGate in the featuregatepromotion package, update
GetPromotionStatus and the install_tests block in the server to use it, and
remove the direct strings.Contains check so both behaviors remain synchronized.
In `@sippy-ng/src/tests/FeatureGateDetail.jsx`:
- Around line 259-277: Extract the duplicated
`/jobs/{release}/analysis?filters=...` URL construction into a shared helper,
preserving the existing matching-job filter item shape and `linkOperator: 'or'`.
Update the “Analyze All” button in `FeatureGateDetail` and the corresponding
flow in `FeatureGatePromotionTab` to import and use this helper.
In `@sippy-ng/src/tests/FeatureGatePromotionTab.jsx`:
- Around line 145-149: Remove the hardcoded thresholds object from the
FeatureGatePromotionTab logic and use the promotion API’s authoritative
validation output instead, preferably rendering data.errors and data.warnings
directly. Ensure sufficient-state messaging stays consistent with the Go
constants RequiredNumberOfTests, RequiredNumberOfTestRunsPerVariant, and
RequiredPassRateOfTestsPerVariant.
- Around line 22-25: Replace the hardcoded PASS_COLOR, PASS_TEXT, FAIL_COLOR,
and FAIL_TEXT constants in FeatureGatePromotionTab with values derived from
Material-UI’s useTheme() palette, matching TestTable’s use of
theme.palette.success.dark and theme.palette.error.dark for the corresponding
cell backgrounds and text colors. Update the affected component flow so these
theme-derived colors are used at all existing pass/fail cell styling sites.
🪄 Autofix
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: Enterprise
Run ID: 08d3f457-3ac5-4eb4-b081-009ccf4c507d
📒 Files selected for processing (15)
cmd/sippy/seed_data.gopkg/api/README.mdpkg/api/featuregatepromotion/filters.gopkg/api/featuregatepromotion/promotion.gopkg/api/featuregatepromotion/promotion_test.gopkg/api/featuregatepromotion/types.gopkg/api/tests.gopkg/apis/api/types.gopkg/db/query/feature_gates.gopkg/sippyserver/server.gopkg/util/param/param.gosippy-ng/src/tests/FeatureGateDetail.jsxsippy-ng/src/tests/FeatureGatePromotionTab.jsxtest/e2e/feature_gate_promotion_test.gotest/e2e/feature_gates_test.go
| // Apply lookback: use 7-day window if sufficient, else extend to 14 days | ||
| if test.CurrentRuns >= RequiredNumberOfTestRunsPerVariant { | ||
| tr.TotalRuns += test.CurrentRuns | ||
| tr.SuccessfulRuns += test.CurrentSuccesses | ||
| tr.FailedRuns += test.CurrentFailures | ||
| tr.FlakedRuns += test.CurrentFlakes | ||
| } else { | ||
| tr.TotalRuns += test.CurrentRuns + test.PreviousRuns | ||
| tr.SuccessfulRuns += test.CurrentSuccesses + test.PreviousSuccesses | ||
| tr.FailedRuns += test.CurrentFailures + test.PreviousFailures | ||
| tr.FlakedRuns += test.CurrentFlakes + test.PreviousFlakes | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The lookback decision is made per row, so one test can mix 7-day and 14-day windows.
allTests can contain several rows for the same test name inside one variant combo, for example one row per JobTier or per suite. The loop evaluates test.CurrentRuns >= RequiredNumberOfTestRunsPerVariant for each row separately and accumulates into the same TestResult. A test with one high-volume row and one low-volume row then gets TotalRuns built from a 7-day window for the first row and a 14-day window for the second. The resulting denominator and pass percentage do not correspond to any single window, and the RequiredNumberOfTestRunsPerVariant check at line 465 can pass or fail for the wrong reason.
Decide the window once per test name. Accumulate current and previous counts separately in the loop, then choose which totals to use after the loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/featuregatepromotion/promotion.go` around lines 425 - 436, Update the
aggregation around the loop building each TestResult so the lookback window is
chosen once per test name rather than per row. Accumulate current and previous
run, success, failure, and flake counts separately for all rows, then select the
7-day totals when aggregate current runs meet
RequiredNumberOfTestRunsPerVariant; otherwise use the combined
current-plus-previous totals. Ensure the resulting TestResult totals and
subsequent threshold check use only the selected window.
| // QueryTestResults queries test results using the same logic as the /api/tests | ||
| // endpoint but without requiring an HTTP request. This allows internal callers | ||
| // (such as promotion readiness) to reuse the exact same query path and filters. | ||
| func QueryTestResults(ctx context.Context, dbc *db.DB, cacheClient cache.Cache, release string, f *filter.Filter) ([]apitype.Test, error) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
State that this helper queries PostgreSQL only.
/api/tests can also serve BigQuery results; the frontend selects the source through the testTableDBSource cookie in sippy-ng/src/tests/TestTable.jsx. This helper always calls buildTestsResultsFromPostgres, so promotion evaluation can differ from what a BigQuery-backed table shows for the same filter. Name the data source in the comment so callers know the guarantee.
📝 Proposed change
-// QueryTestResults queries test results using the same logic as the /api/tests
-// endpoint but without requiring an HTTP request. This allows internal callers
-// (such as promotion readiness) to reuse the exact same query path and filters.
+// QueryTestResults queries test results using the same PostgreSQL logic as the
+// /api/tests endpoint but without requiring an HTTP request. This allows internal
+// callers (such as promotion readiness) to reuse the same query path and filters.
+// The BigQuery path (/api/tests/v2) is not covered.📝 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.
| // QueryTestResults queries test results using the same logic as the /api/tests | |
| // endpoint but without requiring an HTTP request. This allows internal callers | |
| // (such as promotion readiness) to reuse the exact same query path and filters. | |
| func QueryTestResults(ctx context.Context, dbc *db.DB, cacheClient cache.Cache, release string, f *filter.Filter) ([]apitype.Test, error) { | |
| // QueryTestResults queries test results using the same PostgreSQL logic as the | |
| // /api/tests endpoint but without requiring an HTTP request. This allows internal | |
| // callers (such as promotion readiness) to reuse the same query path and filters. | |
| // The BigQuery path (/api/tests/v2) is not covered. | |
| func QueryTestResults(ctx context.Context, dbc *db.DB, cacheClient cache.Cache, release string, f *filter.Filter) ([]apitype.Test, error) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/tests.go` around lines 356 - 359, Update the documentation comment
for QueryTestResults to explicitly state that it queries PostgreSQL only through
buildTestsResultsFromPostgres and does not support BigQuery-backed results, so
callers understand its data-source limitation.
| { | ||
| columnField: 'current_runs', | ||
| operatorValue: '>=', | ||
| value: '0', | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Hand-written filters in the detail view drift from the canonical Go filters. The component re-declares filter definitions that pkg/api/featuregatepromotion/filters.go owns and that the API already publishes through gate.links. Each copy has drifted, so the tables show a different row set than the promotion analysis evaluated. Prefer deriving the tab filters from the returned gate.links payload; until then, correct each copy.
sippy-ng/src/tests/FeatureGateDetail.jsx#L119-L123: change thecurrent_runsfilter value from'0'to'1'to matchCapabilityRegressionsFilter.sippy-ng/src/tests/FeatureGateDetail.jsx#L69-L77: add the twonot: true/has entryitems fornever-stableandaggregated, and setlinkOperator: 'and', to matchGateTestFilter.
📍 Affects 1 file
sippy-ng/src/tests/FeatureGateDetail.jsx#L119-L123(this comment)sippy-ng/src/tests/FeatureGateDetail.jsx#L69-L77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sippy-ng/src/tests/FeatureGateDetail.jsx` around lines 119 - 123, Update the
hand-written filters in FeatureGateDetail: at
sippy-ng/src/tests/FeatureGateDetail.jsx lines 119-123, change the current_runs
value from '0' to '1'; at lines 69-77, add the not: true / has entry conditions
for never-stable and aggregated and set linkOperator to 'and', matching the
canonical GateTestFilter definitions.
| let minRuns = Infinity | ||
| let minPassRate = Infinity | ||
| for (const v of requiredVariants) { | ||
| for (const tr of v.test_results || []) { | ||
| if (tr.total_runs < minRuns) minRuns = tr.total_runs | ||
| if (tr.pass_percent < minPassRate) minPassRate = tr.pass_percent | ||
| } | ||
| } | ||
| if (!isFinite(minRuns)) minRuns = 0 | ||
| if (!isFinite(minPassRate)) minPassRate = 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The pass-rate bullet fires for tests that have no runs.
The scan at lines 158-163 includes every test result of the required variants, including results where total_runs is 0 and pass_percent is 0. minPassRate then becomes 0 and the bullet reports "minimum 0%". The backend does not agree: buildVariantResult in pkg/api/featuregatepromotion/promotion.go line 471 raises a pass-rate error only when tr.TotalRuns > 0. A gate that only lacks run volume therefore shows a pass-rate failure it does not have. Apply the same total_runs > 0 condition here.
🐛 Proposed fix
for (const v of requiredVariants) {
for (const tr of v.test_results || []) {
if (tr.total_runs < minRuns) minRuns = tr.total_runs
- if (tr.pass_percent < minPassRate) minPassRate = tr.pass_percent
+ if (tr.total_runs > 0 && tr.pass_percent < minPassRate) {
+ minPassRate = tr.pass_percent
+ }
}
}Also applies to: 215-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sippy-ng/src/tests/FeatureGatePromotionTab.jsx` around lines 156 - 165,
Update the minPassRate calculation in the requiredVariants scan to consider a
test result’s pass_percent only when its total_runs is greater than zero,
matching buildVariantResult behavior. Apply the same condition in the
corresponding scan around the secondary occurrence, while preserving minRuns
calculation and the existing zero fallback when no eligible results exist.
| func TestFeatureGateDetailNotFound(t *testing.T) { | ||
| var fg api.FeatureGate | ||
| err := util.SippyGet("/api/feature_gates/NonExistentGate12345?release="+util.Release, &fg) | ||
| assert.Error(t, err, "should return error for non-existent gate") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find an e2e helper that exposes the HTTP status code.
set -euo pipefail
fd -t f . test/e2e/util | xargs -r ast-grep outline --items all
rg -nP --type=go -C4 'func SippyGet|StatusCode' test/e2e/utilRepository: openshift/sippy
Length of output: 4442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test/e2e/util/e2erequest.go ---'
sed -n '1,135p' test/e2e/util/e2erequest.go
printf '%s\n' '--- test/e2e/feature_gates_test.go ---'
sed -n '80,125p' test/e2e/feature_gates_test.go
printf '%s\n' '--- server handler and related tests ---'
rg -n -C8 'FeatureGateDetailNotFound|NonExistentGate|StatusNotFound|feature_gates' pkg/sippyserver test -g '*.go' | head -240Repository: openshift/sippy
Length of output: 20025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- feature-gate detail handler ---'
sed -n '700,782p' pkg/sippyserver/server.go
printf '%s\n' '--- status assertions and request-helper usage ---'
rg -n -C3 'sippy API request failed with code|SippyGetAbsolute|http\.StatusNotFound|EqualError|ErrorContains' test pkg -g '*.go' | head -260Repository: openshift/sippy
Length of output: 10218
Assert the expected status code for the missing gate.
SippyGet includes the response status in its error. Replace assert.Error with assert.ErrorContains(t, err, "code 404") to reject other HTTP, transport, or decode errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/feature_gates_test.go` around lines 104 - 108, Update
TestFeatureGateDetailNotFound to assert that the SippyGet error contains “code
404” instead of only checking that an error occurred, ensuring the missing
feature gate returns the expected HTTP status.
…results Remove constant-value parameters from makeTest helper to satisfy unparam linter. Add platform-specific FilterVariants test cases for AWS, GCP, and Azure gates. Initialize TestResults to empty slice in convertPromotionStatus so the API returns [] instead of null for variants with no test data. Remove unused HIDDEN_VARIANT_KEYS and featureGate prop from PromotionCell. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Without this flag, optional platform variants (nutanix, openstack, two-node) were treated as required, causing their errors to block promotion instead of being downgraded to warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The raw SQL query for capability regressions was not excluding aggregated variant jobs, diverging from the canonical CapabilityRegressionsFilter which excludes both never-stable and aggregated variants. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OperatorContains does substring matching, so Capability:Foo would incorrectly match Capability:FooBar. Use OperatorHasEntry for exact array entry matching, consistent with CapabilityRegressionsFilter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests with total_runs=0 have pass_percent=0, which would cause the promotion summary to show a misleading "minimum 0%" pass rate. Match the backend behavior which only reports pass-rate errors when total_runs > 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Without this guard, the test would silently pass if ResultsByVariant were empty, since the loop body would never execute. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Scheduling required tests: |
| } | ||
|
|
||
| var rows []queryResult | ||
| tx := dbc.DB.Raw(` |
There was a problem hiding this comment.
My least favourite part of this PR is this forked query. It was too slow using the normal test functions and filters. This is true if you follow the gate_job_tests hateoas link as well.
| ) | ||
| AND t.name NOT LIKE '%' || ? || '%' | ||
| AND t.name NOT LIKE '%' || ? || '%' | ||
| AND t.name NOT LIKE '%' || ? || '%' |
There was a problem hiding this comment.
When I grilled claude:
⏺ The three lines look identical in the SQL but each ? binds to a different value from excludedNames:
excludedNames := []string{"install should succeed", "openshift-tests should work", "infrastructure should work"}
So it's three distinct filters, one per excluded test name. The SQL just looks repetitive because the pattern t.name NOT LIKE '%' || ? || '%' is the same shape for each. It could be cleaner as a single NOT EXISTS with an unnest or a
NOT (t.name LIKE ANY(...)), but functionally it's correct as-is.
|
Scheduling required tests: |
|
@dgoodwin: 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. |
1 participant
This change unifies sippy's feature gate details page with the logic that was rapidly evolving in the o/api presubmit. Teams will now be able to check their promotion status without opening an o/api PR and waiting for a presubmit to run. The sippy version runs fast enough for live queries. o/api presubmit can use the new API to check validity and just link to this page for results.
New logic includes:
Summary by CodeRabbit
New Features
Documentation
Tests