OCPBUGS-99459: Improve verify-feature-promotion checks for Install featuregates#2943
OCPBUGS-99459: Improve verify-feature-promotion checks for Install featuregates#2943sadasu wants to merge 5 commits into
verify-feature-promotion checks for Install featuregates#2943Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@sadasu: This pull request references Jira Issue OCPBUGS-99459, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@sadasu: This pull request references Jira Issue OCPBUGS-99459, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
openshift-ci-robot
removed
the
jira/invalid-bug
PR Summary by QodoEnhance verify-feature-promotion checks for Install gates and metal (SNO/compact)
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
openshift-ci
Bot
added
the
size/XL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe feature-gate analyzer adds Install-specific aggregation, validation, and reporting for Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review by Qodo
1.
|
![qodo-for-rh-openshift[bot]](https://cdn.github.com/u/792337?s=60&v=4){kind=small}
| // For Install feature gates, validate "install should succeed: overall" test | ||
| if strings.Contains(featureGate, "Install") { | ||
| installTest := testResultByName(testedVariant.TestResults, "install should succeed: overall") | ||
| if installTest == nil { | ||
| results = append(results, ValidationResult{ | ||
| Error: fmt.Errorf("warning: \"install should succeed: overall\" test not found for Install feature gate %q on %v", | ||
| featureGate, jobVariant), | ||
| IsWarning: true, | ||
| }) | ||
| } else { | ||
| if installTest.TotalRuns < requiredNumberOfTestRunsPerVariant { | ||
| results = append(results, ValidationResult{ | ||
| Error: fmt.Errorf("error: \"install should succeed: overall\" only has %d runs, need at least %d runs for %q on %v", | ||
| installTest.TotalRuns, requiredNumberOfTestRunsPerVariant, featureGate, jobVariant), | ||
| IsWarning: isOptional, | ||
| }) | ||
| } | ||
| if installTest.TotalRuns > 0 { | ||
| passPercent := float32(installTest.SuccessfulRuns) / float32(installTest.TotalRuns) | ||
| if passPercent < requiredPassRateForInstallTest { | ||
| displayExpected := int(requiredPassRateForInstallTest * 100) | ||
| displayActual := int(passPercent * 100) | ||
| results = append(results, ValidationResult{ | ||
| Error: fmt.Errorf("error: \"install should succeed: overall\" only passed %d%%, need at least %d%% for %q on %v", | ||
| displayActual, displayExpected, featureGate, jobVariant), | ||
| IsWarning: isOptional, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Install test not validated 🐞 Bug ≡ Correctness
checkIfTestingIsSufficient() looks for "install should succeed: overall" inside testedVariant.TestResults, but Install feature gates use verifyJobBasedFeatureGatePromotion() which populates TestResults with Sippy job names, so the install test is usually missing and only a warning is emitted. The newly fetched installTestLevelData is printed but never fed into validation, making the 100% install requirement ineffective.
Agent Prompt
## Issue description
The new Install-only requirement ("install should succeed: overall" must be 100%) is validated by searching `testedVariant.TestResults`, but Install feature gates use job-based results (`verifyJobBasedFeatureGatePromotion`) where `TestName` is the Sippy **job** name, not the test name. The PR also fetches `installTestLevelData` but only prints it, so the enforcement never applies to the fetched test-level stats.
## Issue Context
`checkIfTestingIsSufficient()` should validate the install-test pass rate using the same data returned by `getInstallTestLevelData()` (or merge that data into `testingResults` in a way that `checkIfTestingIsSufficient()` consumes).
## Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[415-501]
- tools/codegen/cmd/featuregate-test-analyzer.go[848-901]
- tools/codegen/cmd/featuregate-test-analyzer.go[991-1087]
### Suggested approach
- Option A (clean): change `checkIfTestingIsSufficient(featureGate, testingResults, installTestLevelData)` and validate the install test against `installTestLevelData[jobVariant]`.
- Option B: when Install feature gate, inject the returned install test result into `results[jobVariant].TestResults` with `TestName == "install should succeed: overall"` so existing validation finds it.
- Also consider making "test not found" an error for non-optional variants (or at least consistent with `isOptional`), since the requirement is stated as mandatory.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| response, err := sippyClient.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if response.StatusCode < 200 || response.StatusCode > 299 { | ||
| return nil, fmt.Errorf("error getting sippy results (status=%d) for: %v", response.StatusCode, currURL.String()) | ||
| } | ||
| queryResultBytes, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| response.Body.Close() | ||
|
|
There was a problem hiding this comment.
3. Http response body leak 🐞 Bug ☼ Reliability
getInstallTestLevelData() returns on non-2xx responses and on ReadAll errors without closing response.Body, preventing connection reuse and leaking resources. Over multiple variants/tiers, this can exhaust the transport pool and cause timeouts/flaky verify-feature-promotion runs when Sippy misbehaves.
Agent Prompt
## Issue description
In `getInstallTestLevelData()`, `response.Body.Close()` is only called after a successful `io.ReadAll`. On non-2xx responses and `ReadAll` failures, the function returns without closing the body.
## Issue Context
Leaked bodies prevent HTTP keep-alive reuse and can consume resources in the transport, especially when this function is called for many variants and tiers.
## Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[1039-1052]
### Suggested approach
- Immediately after a successful `Do(req)`, add `defer response.Body.Close()`.
- Then remove the manual `response.Body.Close()` call after `ReadAll` (or keep it but ensure it doesn’t double-close).
- Optionally, when status is non-2xx, read and include a small portion of the body in the error for debugging (still with the deferred close).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Always include metal single and compact variants as optional checks | ||
| for _, v := range optionalSelfManagedPlatformVariants { | ||
| if strings.ToLower(v.Cloud) == "metal" && (strings.ToLower(v.Topology) == "single" || strings.ToLower(v.Topology) == "compact") { | ||
| jobVariantsToCheck = append(jobVariantsToCheck, v) | ||
| } | ||
| } |
There was a problem hiding this comment.
4. Duplicate metal variant queries 🐞 Bug ➹ Performance
listTestResultFor() appends platform-filtered variants and then unconditionally appends metal single/compact variants again, which duplicates entries when the feature gate string matches "metal" and filterVariants already selected them. This causes redundant Sippy API calls and extra runtime/load with no behavioral benefit.
Agent Prompt
## Issue description
`listTestResultFor()` can add the same `JobVariant` more than once (append filtered variants, then append metal single/compact again), leading to duplicated Sippy calls.
## Issue Context
`filterVariants()` includes variants when the feature gate string contains the cloud name (e.g. "metal"), so metal single/compact can already be present before the unconditional append.
## Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[858-875]
- tools/codegen/cmd/featuregate-test-analyzer.go[904-918]
### Suggested approach
- Deduplicate `jobVariantsToCheck` before validation/querying (e.g., `map[JobVariant]struct{}` preserving a stable order).
- Or, only append metal single/compact if not already present.
- If these are meant to be non-blocking, also consider setting `Optional: true` on those appended variants (or clarifying the comment).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
187-268: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse one source for the Install stats and validation numbers.
checkIfTestingIsSufficientvalidatestestingResults, but the Install block printsinstallTestLevelDatafrom a separate Sippy query path. That can make the summary counts differ from the blocking/error text and doubles the Sippy calls for the same feature gate.🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 187 - 268, Update the Install statistics block and validation flow around checkIfTestingIsSufficient to use the same test-result data source and counts. Remove the separate installTestLevelData lookup/query path, derive the displayed Install “install should succeed: overall” statistics from testingResults, and preserve the existing validation and output behavior while avoiding duplicate Sippy calls.Source: Linters/SAST tools
🧹 Nitpick comments (1)
tools/codegen/cmd/featuregate-test-analyzer_test.go (1)
484-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage of the new Install validation path; consider adding an
Optional: truecase.All six scenarios correctly exercise run-count/pass-rate thresholds and the missing-test warning. None of them set
Optional: trueon theJobVariant, so theisOptionalbranch in the dedicated Install validation (which turns these same failures into warnings for optional variants) isn't covered 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 `@tools/codegen/cmd/featuregate-test-analyzer_test.go` around lines 484 - 666, The table-driven test Test_checkIfTestingIsSufficient_InstallFeatureGates lacks coverage for optional JobVariant handling. Add a scenario with Optional: true that triggers the Install validation failure, such as an install test below the required pass rate or run count, and assert it produces a warning rather than a blocking error while preserving the existing required-variant cases.
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 686-695: Update the metal amd64 single and compact variants in the
variant definitions used by jobVariantsToCheck to set Optional: true before
appending them, ensuring checkIfTestingIsSufficient treats failures as
non-blocking.
- Around line 991-1081: Update getInstallTestLevelData to maintain separate
TestResults accumulators keyed by currTest.Name, matching the grouping behavior
of listTestResultForVariant. Accumulate each test’s run counters only into its
name-specific result, and return the grouped results without merging distinct
test names into the first-seen entry.
In `@tools/codegen/pkg/sippy/json_types.go`:
- Around line 182-279: Refactor QueriesForWithCapability to delegate shared
query construction to QueriesFor rather than duplicating base filters, optional
network/OS handling, and JobTier parsing. Preserve the existing capability
filter by incorporating it through the delegated query-building path, and retain
the same default, deduplicated tier behavior and returned TierName values.
---
Outside diff comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 187-268: Update the Install statistics block and validation flow
around checkIfTestingIsSufficient to use the same test-result data source and
counts. Remove the separate installTestLevelData lookup/query path, derive the
displayed Install “install should succeed: overall” statistics from
testingResults, and preserve the existing validation and output behavior while
avoiding duplicate Sippy calls.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer_test.go`:
- Around line 484-666: The table-driven test
Test_checkIfTestingIsSufficient_InstallFeatureGates lacks coverage for optional
JobVariant handling. Add a scenario with Optional: true that triggers the
Install validation failure, such as an install test below the required pass rate
or run count, and assert it produces a warning rather than a blocking error
while preserving the existing required-variant cases.
🪄 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: Enterprise
Run ID: 6720eb14-cba1-4786-b953-c54d5cf31cf4
📒 Files selected for processing (3)
tools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/cmd/featuregate-test-analyzer_test.gotools/codegen/pkg/sippy/json_types.go
| func getInstallTestLevelData(featureGate string, jobVariant JobVariant) (*TestResults, error) { | ||
| testPattern := "install should succeed: overall" | ||
| queries := sippy.QueriesForWithCapability(jobVariant.Cloud, jobVariant.Architecture, jobVariant.Topology, | ||
| jobVariant.NetworkStack, jobVariant.OS, jobVariant.JobTiers, testPattern, featureGate) | ||
|
|
||
| defaultTransport := &http.Transport{ | ||
| Proxy: http.ProxyFromEnvironment, | ||
| ForceAttemptHTTP2: true, | ||
| MaxIdleConns: 100, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| ExpectContinueTimeout: 1 * time.Second, | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| }, | ||
| } | ||
|
|
||
| sippyClient := &http.Client{ | ||
| Timeout: 2 * time.Minute, | ||
| Transport: defaultTransport, | ||
| } | ||
|
|
||
| release, err := getRelease() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("couldn't fetch latest release version: %w", err) | ||
| } | ||
|
|
||
| var installTestResult *TestResults | ||
| for _, currQuery := range queries { | ||
| currURL := &url.URL{ | ||
| Scheme: "https", | ||
| Host: "sippy.dptools.openshift.org", | ||
| Path: "api/tests", | ||
| } | ||
| queryParams := currURL.Query() | ||
| queryParams.Add("release", release) | ||
| queryParams.Add("period", "default") | ||
| filterJSON, err := json.Marshal(currQuery) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| queryParams.Add("filter", string(filterJSON)) | ||
| currURL.RawQuery = queryParams.Encode() | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, currURL.String(), nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| response, err := sippyClient.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if response.StatusCode < 200 || response.StatusCode > 299 { | ||
| return nil, fmt.Errorf("error getting sippy results (status=%d) for: %v", response.StatusCode, currURL.String()) | ||
| } | ||
| queryResultBytes, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| response.Body.Close() | ||
|
|
||
| testInfos := []sippy.SippyTestInfo{} | ||
| if err := json.Unmarshal(queryResultBytes, &testInfos); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, currTest := range testInfos { | ||
| if installTestResult == nil { | ||
| installTestResult = &TestResults{ | ||
| TestName: currTest.Name, | ||
| } | ||
| } | ||
|
|
||
| // Accumulate results across multiple JobTier queries | ||
| if currTest.CurrentRuns >= requiredNumberOfTestRunsPerVariant { | ||
| installTestResult.TotalRuns += currTest.CurrentRuns | ||
| installTestResult.SuccessfulRuns += currTest.CurrentSuccesses | ||
| installTestResult.FailedRuns += currTest.CurrentFailures | ||
| installTestResult.FlakedRuns += currTest.CurrentFlakes | ||
| } else { | ||
| installTestResult.TotalRuns += currTest.CurrentRuns + currTest.PreviousRuns | ||
| installTestResult.SuccessfulRuns += currTest.CurrentSuccesses + currTest.PreviousSuccesses | ||
| installTestResult.FailedRuns += currTest.CurrentFailures + currTest.PreviousFailures | ||
| installTestResult.FlakedRuns += currTest.CurrentFlakes + currTest.PreviousFlakes | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return installTestResult, nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='tools/codegen/cmd/featuregate-test-analyzer.go'
echo '--- outline ---'
ast-grep outline "$file" --view expanded || true
echo
echo '--- symbols around listTestResultForVariant / getInstallTestLevelData ---'
rg -n "func listTestResultForVariant|func getInstallTestLevelData|QueriesForWithCapability|CurrentRuns|TestName|context.Context|InsecureSkipVerify" "$file"
echo
echo '--- relevant slices ---'
sed -n '860,1125p' "$file"Repository: openshift/api
Length of output: 15074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='tools/codegen/cmd/featuregate-test-analyzer.go'
echo '--- listTestResultForVariant body ---'
sed -n '1083,1245p' "$file"
echo
echo '--- name/pattern handling ---'
rg -n "install should succeed: overall|QueriesForWithCapability|QueriesFor\\(|testResultByName\\(|map\\[string\\]\\*TestResults|strings.Contains\\(featureGate, \"Install\"\\)" "$file"Repository: openshift/api
Length of output: 7771
🌐 Web query:
Sippy API /api/tests filter "name contains" query returns multiple test names QueryForWithCapability install should succeed overall
💡 Result:
In the context of the OpenShift Sippy testing dashboard, the /api/tests endpoint supports advanced filtering using JSON-formatted parameters [1]. When using the "contains" operator with a "name" column, the API is designed to return all test names that match the specified substring criteria [1]. If a query like "QueryForWithCapability" is passed as a filter, the system will return all test records where the name field contains that string. If multiple tests contain that sequence, all matching entries will be returned in the API response, which is the expected behavior for this endpoint [1]. There is no native "QueryForWithCapability" function or specific hardcoded behavior that restricts the result to a single test name; rather, the filter logic acts as a standard substring search [1]. If "QueryForWithCapability" is intended to be a specific, singular test name, ensure the filter is precise. If your intent is to confirm why multiple names are returned, it is because the "contains" operator is performing a partial match across the available test dataset [1]. To refine your query: - Ensure you are using the correct JSON filter format as defined in the Sippy API documentation [1]. - If you require a specific test, consider if a more restrictive operator (like an "equals" check if supported) or a different column filter (e.g., matching a unique ID) is more appropriate than "contains" [1]. Please note that Sippy's API filtering follows a structured format [1]: { "columnName": "name", "operatorValue": "contains", "value": "QueryForWithCapability" } If the system returns multiple results, it confirms that multiple tests contain that string in their name [1].
Citations:
Bucket install results by test name getInstallTestLevelData should group testInfos by currTest.Name like listTestResultForVariant; Sippy can return multiple matching names, and this single accumulator will silently merge them into the first-seen result.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 995-1005: This http.Transport is configured with a tls.Config that sets InsecureSkipVerify: true, which disables TLS certificate verification for every request made through the resulting http.Client. The server's certificate chain and host name are not validated, exposing the connection to man-in-the-middle attacks. Remove InsecureSkipVerify (or set it to false) and supply a proper RootCAs pool if you need to trust custom certificates.
Context: http.Transport{
Proxy: http.ProxyFromEnvironment,
ForceAttemptHTTP2: true,
MaxIdleConns: 100,
IdleConnTimeout: 90 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
},
}
Note: [CWE-295] Improper Certificate Validation.
(http-transport-tls-skip-verify-go)
[warning] 1002-1004: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(missing-ssl-minversion-go)
🪛 OpenGrep (1.25.0)
[ERROR] 1003-1005: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 1003-1005: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 991 - 1081,
Update getInstallTestLevelData to maintain separate TestResults accumulators
keyed by currTest.Name, matching the grouping behavior of
listTestResultForVariant. Accumulate each test’s run counters only into its
name-specific result, and return the grouped results without merging distinct
test names into the first-seen entry.
| func QueriesForWithCapability(cloud, architecture, topology, networkStack, os, jobTiers, testPattern, capability string) []*SippyQueryStruct { | ||
| // Build base query items that are common to all JobTier queries | ||
| baseItems := []SippyQueryItem{ | ||
| { | ||
| ColumnField: "variants", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("Platform:%s", cloud), | ||
| }, | ||
| { | ||
| ColumnField: "variants", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("Architecture:%s", architecture), | ||
| }, | ||
| { | ||
| ColumnField: "variants", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("Topology:%s", topology), | ||
| }, | ||
| { | ||
| ColumnField: "name", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: testPattern, | ||
| }, | ||
| { | ||
| ColumnField: "variants", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("Capability:%s", capability), | ||
| }, | ||
| } | ||
|
|
||
| if networkStack != "" { | ||
| baseItems = append(baseItems, SippyQueryItem{ | ||
| ColumnField: "variants", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("NetworkStack:%s", networkStack), | ||
| }) | ||
| } | ||
|
|
||
| if os != "" { | ||
| baseItems = append(baseItems, SippyQueryItem{ | ||
| ColumnField: "variants", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("OS:%s", os), | ||
| }) | ||
| } | ||
|
|
||
| // Parse JobTiers - comma-separated string, default to standard/informing/blocking/candidate if empty | ||
| var jobTiersList []string | ||
| if jobTiers == "" { | ||
| jobTiersList = []string{"standard", "informing", "blocking", "candidate"} | ||
| } else { | ||
| // Split by comma, trim whitespace, and deduplicate using sets | ||
| tierSet := sets.New[string]() | ||
| for _, tier := range strings.Split(jobTiers, ",") { | ||
| if trimmed := strings.TrimSpace(tier); trimmed != "" { | ||
| tierSet.Insert(trimmed) | ||
| } | ||
| } | ||
| // If all tiers were whitespace/empty after trimming, use defaults | ||
| if tierSet.Len() == 0 { | ||
| jobTiersList = []string{"standard", "informing", "blocking", "candidate"} | ||
| } else { | ||
| jobTiersList = sets.List(tierSet) | ||
| } | ||
| } | ||
|
|
||
| // Generate one query per JobTier (to work around API's single LinkOperator limitation) | ||
| var queries []*SippyQueryStruct | ||
| for _, jobTier := range jobTiersList { | ||
| // Copy base items for this query | ||
| items := make([]SippyQueryItem, len(baseItems)) | ||
| copy(items, baseItems) | ||
|
|
||
| // Add JobTier filter | ||
| items = append(items, SippyQueryItem{ | ||
| ColumnField: "variants", | ||
| Not: false, | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("JobTier:%s", jobTier), | ||
| }) | ||
|
|
||
| queries = append(queries, &SippyQueryStruct{ | ||
| Items: items, | ||
| LinkOperator: "and", | ||
| TierName: jobTier, | ||
| }) | ||
| } | ||
|
|
||
| return queries | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Delegate to QueriesFor instead of duplicating ~90 lines.
QueriesForWithCapability is an almost byte-for-byte copy of QueriesFor (tier parsing, defaults, network/OS filters) with only one extra item appended. Any future fix/change to tier parsing or defaults must now be made in two places, risking drift.
♻️ Proposed refactor
func QueriesForWithCapability(cloud, architecture, topology, networkStack, os, jobTiers, testPattern, capability string) []*SippyQueryStruct {
- // Build base query items that are common to all JobTier queries
- baseItems := []SippyQueryItem{
- ...
- }
-
- if networkStack != "" { ... }
- if os != "" { ... }
-
- // Parse JobTiers ...
- var jobTiersList []string
- ...
-
- var queries []*SippyQueryStruct
- for _, jobTier := range jobTiersList {
- ...
- }
-
- return queries
+ queries := QueriesFor(cloud, architecture, topology, networkStack, os, jobTiers, testPattern)
+ capabilityItem := SippyQueryItem{
+ ColumnField: "variants",
+ Not: false,
+ OperatorValue: "contains",
+ Value: fmt.Sprintf("Capability:%s", capability),
+ }
+ for _, q := range queries {
+ q.Items = append(q.Items, capabilityItem)
+ }
+ return queries
}📝 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.
| func QueriesForWithCapability(cloud, architecture, topology, networkStack, os, jobTiers, testPattern, capability string) []*SippyQueryStruct { | |
| // Build base query items that are common to all JobTier queries | |
| baseItems := []SippyQueryItem{ | |
| { | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("Platform:%s", cloud), | |
| }, | |
| { | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("Architecture:%s", architecture), | |
| }, | |
| { | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("Topology:%s", topology), | |
| }, | |
| { | |
| ColumnField: "name", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: testPattern, | |
| }, | |
| { | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("Capability:%s", capability), | |
| }, | |
| } | |
| if networkStack != "" { | |
| baseItems = append(baseItems, SippyQueryItem{ | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("NetworkStack:%s", networkStack), | |
| }) | |
| } | |
| if os != "" { | |
| baseItems = append(baseItems, SippyQueryItem{ | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("OS:%s", os), | |
| }) | |
| } | |
| // Parse JobTiers - comma-separated string, default to standard/informing/blocking/candidate if empty | |
| var jobTiersList []string | |
| if jobTiers == "" { | |
| jobTiersList = []string{"standard", "informing", "blocking", "candidate"} | |
| } else { | |
| // Split by comma, trim whitespace, and deduplicate using sets | |
| tierSet := sets.New[string]() | |
| for _, tier := range strings.Split(jobTiers, ",") { | |
| if trimmed := strings.TrimSpace(tier); trimmed != "" { | |
| tierSet.Insert(trimmed) | |
| } | |
| } | |
| // If all tiers were whitespace/empty after trimming, use defaults | |
| if tierSet.Len() == 0 { | |
| jobTiersList = []string{"standard", "informing", "blocking", "candidate"} | |
| } else { | |
| jobTiersList = sets.List(tierSet) | |
| } | |
| } | |
| // Generate one query per JobTier (to work around API's single LinkOperator limitation) | |
| var queries []*SippyQueryStruct | |
| for _, jobTier := range jobTiersList { | |
| // Copy base items for this query | |
| items := make([]SippyQueryItem, len(baseItems)) | |
| copy(items, baseItems) | |
| // Add JobTier filter | |
| items = append(items, SippyQueryItem{ | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("JobTier:%s", jobTier), | |
| }) | |
| queries = append(queries, &SippyQueryStruct{ | |
| Items: items, | |
| LinkOperator: "and", | |
| TierName: jobTier, | |
| }) | |
| } | |
| return queries | |
| } | |
| func QueriesForWithCapability(cloud, architecture, topology, networkStack, os, jobTiers, testPattern, capability string) []*SippyQueryStruct { | |
| queries := QueriesFor(cloud, architecture, topology, networkStack, os, jobTiers, testPattern) | |
| capabilityItem := SippyQueryItem{ | |
| ColumnField: "variants", | |
| Not: false, | |
| OperatorValue: "contains", | |
| Value: fmt.Sprintf("Capability:%s", capability), | |
| } | |
| for _, q := range queries { | |
| q.Items = append(q.Items, capabilityItem) | |
| } | |
| return queries | |
| } |
🤖 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 `@tools/codegen/pkg/sippy/json_types.go` around lines 182 - 279, Refactor
QueriesForWithCapability to delegate shared query construction to QueriesFor
rather than duplicating base filters, optional network/OS handling, and JobTier
parsing. Preserve the existing capability filter by incorporating it through the
delegated query-building path, and retain the same default, deduplicated tier
behavior and returned TierName values.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
1101-1112: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the response body on every exit path
response.Bodyis only closed afterio.ReadAll, so the non-2xx return and aReadAllerror leak the underlying connection. Movedefer response.Body.Close()immediately afterDosucceeds.🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 1101 - 1112, Update the response handling after sippyClient.Do succeeds to defer response.Body.Close() immediately, before the status-code validation and io.ReadAll calls, ensuring every subsequent return path closes the body; remove the later explicit close if it becomes redundant.
🧹 Nitpick comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
1057-1072: 🚀 Performance & Scalability | 🔵 TrivialConsider reusing the shared
*http.Clientpattern used elsewhere instead of constructing a new client per call.
verifyJobPassRate,getJobRunsFromSippy, andgetTriagedTestFailuresFromSippyall accept a shared*http.Clientparameter, butgetInstallTestLevelDatabuilds its ownhttp.Transport/http.Clienton every invocation (per job variant, per feature gate). This duplicates the TLS configuration (including the insecure setting flagged above) and forgoes connection-pool reuse across calls.
[recommended_refactor]🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 1057 - 1072, The getInstallTestLevelData flow should reuse the shared *http.Client pattern already passed to verifyJobPassRate, getJobRunsFromSippy, and getTriagedTestFailuresFromSippy. Remove its per-invocation defaultTransport and sippyClient construction, update the function to accept and use the shared client, and propagate that client through its callers so connection pooling and centralized TLS configuration are preserved.
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 517-536: Remove the blank fmt.Errorf separator from
groupErrorsByCategory so its returned slice contains only actual
ValidationResult errors. Keep category grouping order intact, and rely
exclusively on writeGroupedValidationResults for display spacing via
md.Text("").
---
Outside diff comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1101-1112: Update the response handling after sippyClient.Do
succeeds to defer response.Body.Close() immediately, before the status-code
validation and io.ReadAll calls, ensuring every subsequent return path closes
the body; remove the later explicit close if it becomes redundant.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1057-1072: The getInstallTestLevelData flow should reuse the
shared *http.Client pattern already passed to verifyJobPassRate,
getJobRunsFromSippy, and getTriagedTestFailuresFromSippy. Remove its
per-invocation defaultTransport and sippyClient construction, update the
function to accept and use the shared client, and propagate that client through
its callers so connection pooling and centralized TLS configuration are
preserved.
🪄 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: Enterprise
Run ID: 1b715e85-82e6-4833-9508-1fc141a57057
📒 Files selected for processing (1)
tools/codegen/cmd/featuregate-test-analyzer.go
d7fce23 to
b31e16a
Compare
Augment `verify-feature-promotion` output to indicate pass percentage for `install should succeed` tests for featuregates that include "Install" in their name. This update gives a better indication of whether Install features are failing at installation or later during execution of e2e conformance tests. This update does not change the criteria for reporting success but adds more information in the output for easier analysis of feature state.
Ignore runs with internal and external infrastrcuture failures while calculating pass percentages. The fix does the following: 1. Fetches all jobs for the feature gate variant 2. Fetches individual runs for each job 3. Skips any run with OverallResult == "N" or "n" (infra failures) entirely 4. For remaining runs: checks if "install should succeed: overall" is in FailedTestNames — if not, it's a success
Install feature gates require entirely new jobs to be created that execise the new feature. New tests may be added and existing tests are usually updated to account for the new feature. Adjust the feature gate promotion verification code to account for that.
b31e16a to
3133041
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
747-756: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMetal single/compact variants still aren't marked
Optional: true— same finding as a previous review round, still unresolved.The struct literals added at Lines 747-756 don't set
Optional: true, and the loop at Lines 941-946 appends them straight intojobVariantsToCheckunconditionally for every self-managed feature gate (not just Install ones). SincecheckIfTestingIsSufficientderives blocking/warning status purely fromjobVariant.Optional(Line 424), any feature gate that happens to have partial (1-4, i.e. non-zero but belowrequiredNumberOfTests) test coverage on metal single/compact will now produce a blocking error instead of the intended non-blocking check, for variants that previously weren't checked at all.🐛 Proposed fix
{ Cloud: "metal", Architecture: "amd64", Topology: "single", + Optional: true, }, { Cloud: "metal", Architecture: "amd64", Topology: "compact", + Optional: true, },Also applies to: 940-947
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 747 - 756, Mark the metal amd64 single and compact variant literals as Optional so partial coverage remains non-blocking. Update the corresponding entries appended by the self-managed feature-gate loop near jobVariantsToCheck, ensuring both variants retain Optional: true before checkIfTestingIsSufficient evaluates them.
🧹 Nitpick comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
1064-1217: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNew Sippy-calling functions don't accept
context.Contextor useNewRequestWithContext.
getInstallTestResultsFromJobRunsandgetInstallTestLevelDataare new functions that issue outbound HTTP calls but take nocontext.Contextand build requests with plainhttp.NewRequest/client.Get, so the caller's context (available inRun(ctx context.Context)) can't cancel or bound these calls beyond the client's blanket 2-minute timeout.As per path instructions, "
**/*.go: ... context.Context for cancellation and timeouts."🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 1064 - 1217, Update getInstallTestResultsFromJobRuns and getInstallTestLevelData to accept a context.Context parameter, propagate it from their callers, and construct every outbound request with http.NewRequestWithContext using that context. Ensure all Sippy HTTP calls honor caller cancellation and deadlines instead of relying only on the client timeout.Source: Path instructions
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1132-1142: Update the Sippy HTTP client setup shared by
listTestResultForVariant, getInstallTestResultsFromJobRuns, and
getInstallTestLevelData by extracting the duplicated http.Transport/tls.Config
construction into a newSippyClient helper and removing InsecureSkipVerify so
standard TLS certificate validation remains enabled. In each response-handling
path, close response.Body immediately after receiving the response, before
status checks or io.ReadAll, while preserving existing error returns.
- Around line 487-525: Update the missing install-test-data branch in the
Install handling around installTestLevelData[jobVariant] to set IsWarning from
isOptional instead of hardcoding false. Preserve the existing error message and
CategoryInstallTest behavior.
- Line 418: Update all three test call sites of checkIfTestingIsSufficient in
featuregate-test-analyzer_test.go to pass the appropriate installTestLevelData
argument as the third parameter, matching the function’s new signature and
preserving each test’s existing data setup.
---
Duplicate comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 747-756: Mark the metal amd64 single and compact variant literals
as Optional so partial coverage remains non-blocking. Update the corresponding
entries appended by the self-managed feature-gate loop near jobVariantsToCheck,
ensuring both variants retain Optional: true before checkIfTestingIsSufficient
evaluates them.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1064-1217: Update getInstallTestResultsFromJobRuns and
getInstallTestLevelData to accept a context.Context parameter, propagate it from
their callers, and construct every outbound request with
http.NewRequestWithContext using that context. Ensure all Sippy HTTP calls honor
caller cancellation and deadlines instead of relying only on the client timeout.
🪄 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: Enterprise
Run ID: a5051f31-3812-471b-8aa4-d7c79d6ce778
📒 Files selected for processing (4)
tools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/cmd/featuregate-test-analyzer_test.gotools/codegen/cmd/root.gotools/codegen/pkg/sippy/json_types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/codegen/cmd/featuregate-test-analyzer_test.go
- tools/codegen/pkg/sippy/json_types.go
sadasu
changed the title
verify-feature-promotion checksverify-feature-promotion checks for Install featuregates
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/codegen/cmd/featuregate-test-analyzer.go (2)
272-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not label every warning as an optional-variant issue.
Candidate-tier warnings and Install informational findings are emitted for required variants too, so this heading misstates their cause. Use a neutral label such as “Non-blocking warnings and informational results.”
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 272 - 275, Update the warning section heading in the len(warnings) block of the feature-gate analysis flow to use neutral wording such as “Non-blocking warnings and informational results,” rather than attributing all entries to optional variants. Keep the existing writeGroupedValidationResults(warningResults, md) behavior unchanged.
186-188: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
ctxinto the Sippy fetch path.Runalready accepts cancellation, butlistTestResultForand the downstream Sippy helpers still create plain requests and rely only on the per-client timeout. A cancelled run can keep making Sippy calls for up to 2 minutes per request.🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 186 - 188, Update Run’s Sippy fetch path so the existing ctx is passed through listTestResultFor and every downstream Sippy helper, using context-aware request creation instead of plain requests. Preserve the current results and error propagation while ensuring cancellation stops in-flight and subsequent Sippy calls promptly.Source: Path instructions
🤖 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.
Outside diff comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 272-275: Update the warning section heading in the len(warnings)
block of the feature-gate analysis flow to use neutral wording such as
“Non-blocking warnings and informational results,” rather than attributing all
entries to optional variants. Keep the existing
writeGroupedValidationResults(warningResults, md) behavior unchanged.
- Around line 186-188: Update Run’s Sippy fetch path so the existing ctx is
passed through listTestResultFor and every downstream Sippy helper, using
context-aware request creation instead of plain requests. Preserve the current
results and error propagation while ensuring cancellation stops in-flight and
subsequent Sippy calls promptly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 444853aa-8a40-44d5-941d-26f6ea4d57ef
📒 Files selected for processing (2)
tools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/cmd/featuregate-test-analyzer_test.go
574bc88 to
b218be2
Compare
|
@sadasu: The following test failed, say
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
No description provided.