no-jira: Update featuregate-test-analyzer output for Install features#2937
no-jira: Update featuregate-test-analyzer output for Install features#2937sadasu wants to merge 1 commit into
featuregate-test-analyzer output for Install features#2937Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request explicitly references no jira issue. 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. |
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
|
[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 |
📝 WalkthroughWalkthroughInstall feature gates now retrieve and aggregate Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_test.go`:
- Around line 647-653: The test’s warning counting in the results loop does not
verify the reported pass percentage. Update the scenario assertions around the
feature-gate analyzer test to require the expected informational text, including
the percentage and successful/total run counts such as “95% (19/20 runs)”, while
retaining the existing message-count checks.
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 432-435: Successful install telemetry is incorrectly classified as
a warning. Update the ValidationResult handling around the “install should
succeed” result in the feature-gate analyzer to use a distinct informational
classification and rendering path, preserving successful validation while
displaying the pass percentage; then adjust the related warning-count
expectations in the tests.
🪄 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: 29c7df4c-01df-4008-8ce9-c1298947a3c6
📒 Files selected for processing (2)
tools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/cmd/featuregate-test-analyzer_test.go
d6ff78a to
eb66e60
Compare
openshift-ci
Bot
added
the
size/L
ac53575 to
eb66e60
Compare
|
@sadasu can you add a feature promotion example that demonstrates this working correctly? We don't need to merge like that but having a proof through CI would be helpful |
eb66e60 to
8edc6a8
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.
8edc6a8 to
f78c433
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tools/codegen/cmd/featuregate-test-analyzer_test.go (1)
484-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover optional Install variants.
The new validation branches downgrade insufficient runs and sub-100% pass rates when
JobVariant.Optionalis true, but every table entry uses a required variant. Add optional cases for both branches to protect the warning behavior.🤖 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 - 665, Extend Test_checkIfTestingIsSufficient_InstallFeatureGates with table cases using JobVariant.Optional: true that exercise both insufficient install-test runs and a sub-100% install pass rate. Assert each produces a warning rather than a blocking error, while preserving the existing required-variant coverage and expectations.
🤖 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 996-1007: Update the response handling around sippyClient.Do to
defer closing response.Body immediately after a successful request, before
status validation or io.ReadAll. Ensure the deferred cleanup surfaces any close
error rather than discarding it, while preserving the existing non-2xx and
read-error returns.
- Around line 991-999: Thread the context parameter from Run through the
Sippy-fetching helper to the request construction near http.NewRequest, and
replace http.NewRequest with http.NewRequestWithContext using ctx. Preserve the
existing request URL, method, error handling, and response processing while
ensuring cancellation from Run stops in-flight Sippy requests.
- Around line 952-967: Restore certificate verification in the Sippy client
transport by removing the InsecureSkipVerify override from the tls.Config used
by defaultTransport, or configure it with the trusted Sippy CA bundle. Keep the
existing http.Client and transport settings unchanged.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer_test.go`:
- Around line 484-665: Extend
Test_checkIfTestingIsSufficient_InstallFeatureGates with table cases using
JobVariant.Optional: true that exercise both insufficient install-test runs and
a sub-100% install pass rate. Assert each produces a warning rather than a
blocking error, while preserving the existing required-variant coverage and
expectations.
🪄 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: 50cdc048-7fab-411a-8b2e-7ad6445101cc
📒 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
| 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, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first.
ast-grep outline tools/codegen/cmd/featuregate-test-analyzer.go --view expanded > /tmp/outline.txt
sed -n '1,200p' /tmp/outline.txt
# Read the relevant section around the transport/client setup.
sed -n '900,1040p' tools/codegen/cmd/featuregate-test-analyzer.go
# Find where sippyClient or defaultTransport is used.
rg -n "sippyClient|defaultTransport|InsecureSkipVerify|TLSClientConfig|ProxyFromEnvironment" tools/codegen/cmd/featuregate-test-analyzer.goRepository: openshift/api
Length of output: 9751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant slice with line numbers.
nl -ba tools/codegen/cmd/featuregate-test-analyzer.go | sed -n '930,1025p'
# Locate all references to Sippy-related networking in the repo.
rg -n "sippy|InsecureSkipVerify|TLSClientConfig" tools/codegen/cmd -g '!**/*_test.go'Repository: openshift/api
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('tools/codegen/cmd/featuregate-test-analyzer.go')
text = p.read_text()
for key in ['InsecureSkipVerify', 'sippyClient', 'defaultTransport']:
print(f'## {key}')
for i, line in enumerate(text.splitlines(), 1):
if key in line:
start = max(1, i-8)
end = min(len(text.splitlines()), i+12)
for j in range(start, end+1):
print(f'{j:4d}: {text.splitlines()[j-1]}')
print()
break
PYRepository: openshift/api
Length of output: 2425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any trusted-root configuration or explanatory comments near Sippy clients.
rg -n "RootCAs|InsecureSkipVerify|sippy\.dptools\.openshift\.org|crypto/x509|x509\.NewCertPool|AppendCertsFromPEM" tools/codegen/cmd tools -g '!**/*_test.go'
# Show the neighboring code around the other two Sippy client setups in this file.
python3 - <<'PY'
from pathlib import Path
text = Path('tools/codegen/cmd/featuregate-test-analyzer.go').read_text().splitlines()
for anchor in [1054, 1175]:
print(f"\n## around line {anchor}")
for j in range(anchor-6, anchor+18):
if 1 <= j <= len(text):
print(f"{j:4d}: {text[j-1]}")
PYRepository: openshift/api
Length of output: 18642
Restore TLS verification for the Sippy clients. InsecureSkipVerify: true leaves these http.Transport setups open to on-path forgery of Sippy telemetry, which can skew promotion decisions. Remove the override or load the trusted Sippy CA bundle instead.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 958-960: 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] 959-961: 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] 959-961: 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 952 - 967,
Restore certificate verification in the Sippy client transport by removing the
InsecureSkipVerify override from the tls.Config used by defaultTransport, or
configure it with the trusted Sippy CA bundle. Keep the existing http.Client and
transport settings unchanged.
Source: Linters/SAST tools
| 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 | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tools/codegen/cmd/featuregate-test-analyzer.go"
echo "== File size =="
wc -l "$file"
echo "== Around cited lines =="
sed -n '940,1035p' "$file"
echo "== Search for request construction and Sippy calls =="
rg -n "NewRequestWithContext|NewRequest\\(|sippyClient\\.Do\\(|ctx\\b|context\\.Context" "$file"
echo "== Function signatures in file =="
ast-grep outline "$file" --view expandedRepository: openshift/api
Length of output: 8150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tools/codegen/cmd/featuregate-test-analyzer.go"
echo "== getInstallTestLevelData callers =="
rg -n "getInstallTestLevelData\\(" "$file" -A 3 -B 3
echo "== lines around second request site =="
sed -n '1030,1115p' "$file"
echo "== Run path and cancellation handling =="
sed -n '120,170p' "$file"
echo "== any explicit ctx checks or deadlines in request helpers =="
rg -n "context\\.With|ctx\\.Done|select \\{|NewRequestWithContext|http\\.Client\\{.*Timeout|Timeout:" "$file" -A 2 -B 2Repository: openshift/api
Length of output: 7318
Propagate ctx into the Sippy requests. In tools/codegen/cmd/featuregate-test-analyzer.go:947-999, these calls use http.NewRequest, so cancellation from Run won't stop the Sippy fetches and each one can keep running until the 2m client timeout. Thread ctx through the helper and switch to http.NewRequestWithContext.
🤖 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 - 999,
Thread the context parameter from Run through the Sippy-fetching helper to the
request construction near http.NewRequest, and replace http.NewRequest with
http.NewRequestWithContext using ctx. Preserve the existing request URL, method,
error handling, and response processing while ensuring cancellation from Run
stops in-flight Sippy requests.
Source: Path instructions
| 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.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tools/codegen/cmd/featuregate-test-analyzer.go"
echo "== surrounding lines =="
sed -n '960,1035p' "$file" | cat -n
echo
echo "== body close usages in file =="
rg -n "response\.Body\.Close|defer .*Body\.Close|Close\(\)" "$file"Repository: openshift/api
Length of output: 3188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tools/codegen/cmd/featuregate-test-analyzer.go"
printf '== surrounding lines ==\n'
sed -n '960,1035p' "$file" | cat -n
printf '\n== body close usages in file ==\n'
rg -n "response\.Body\.Close|defer .*Body\.Close|Close\(\)" "$file" || trueRepository: openshift/api
Length of output: 3188
Close response.Body on every path
response.Body.Close() only runs after ReadAll; move it to a defer immediately after Do so the non-2xx and read-error returns also clean up, and surface the close error instead of dropping 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 996 - 1007,
Update the response handling around sippyClient.Do to defer closing
response.Body immediately after a successful request, before status validation
or io.ReadAll. Ensure the deferred cleanup surfaces any close error rather than
discarding it, while preserving the existing non-2xx and read-error returns.
Source: Path instructions
openshift-ci
Bot
added
size/XL
|
@JoelSpeed I added this commit to #2797 so it can be tested against actual feature promotion code. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tools/codegen/cmd/featuregate-test-analyzer.go (3)
966-981: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRestore TLS verification for this Sippy client (same unresolved finding as before).
InsecureSkipVerify: truedisables certificate validation forgetInstallTestLevelData's Sippy HTTP client, exposing install telemetry fetches to on-path forgery. This exact pattern was already flagged in a previous review round at this same line range and remains unresolved. This block also duplicates thedefaultTransport/sippyClientconstruction found inlistTestResultForVariant(1068-1083) verbatim — consider extracting a sharednewSippyClient()helper (without the insecure override) to fix the security issue in one place instead of two.🔒 Proposed fix
- 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, - } + sippyClient := newSippyClient()🤖 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 966 - 981, Restore certificate verification in the Sippy HTTP client construction used by getInstallTestLevelData by removing the InsecureSkipVerify override or setting it to false. Also consolidate the duplicated defaultTransport and sippyClient setup shared with listTestResultForVariant into a newSippyClient helper, ensuring both callers use the secure configuration.
1005-1013: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThread
ctxinto these Sippy requests (same unresolved finding as before).
getInstallTestLevelDatauseshttp.NewRequestrather thanhttp.NewRequestWithContext, so cancellation fromRun(ctx)cannot stop in-flight Sippy fetches, which can keep running until the 2‑minute client timeout. This was already flagged at this exact line range in a previous review round.🤖 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 1005 - 1013, Update the Sippy request construction in getInstallTestLevelData to use http.NewRequestWithContext with the ctx received from Run, preserving the existing GET URL and error handling so Run cancellation stops in-flight fetches.
1010-1021: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose
response.Bodyon every path, and don't discard the close error (same unresolved finding as before).
response.Body.Close()only runs after a successfulReadAll; the non-2xx return at line 1015-1016 leaks the body. Move the close to adeferright aftersippyClient.Do(req)so it runs on every path, and don't silently drop its error. As per path instructions, Go code should never ignore error returns.🩹 Proposed fix
response, err := sippyClient.Do(req) if err != nil { return nil, err } + defer func() { + if cerr := response.Body.Close(); cerr != nil { + fmt.Printf("warning: failed to close sippy response body: %v\n", cerr) + } + }() 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()🤖 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 1010 - 1021, Update the response handling in the sippy request flow around sippyClient.Do(req) to defer response.Body.Close() immediately after a successful response is received, ensuring it runs for both non-2xx and read failures. Preserve the existing status and read error returns, but handle and propagate the close error rather than discarding it.Source: Path instructions
🧹 Nitpick comments (4)
tools/codegen/pkg/sippy/json_types.go (1)
182-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared query builder.
Lines 182-278 duplicate
QueriesForexcept for one capability filter. Move the common construction into a private helper with an optional capability argument, leaving both exported functions as thin wrappers; otherwise future filter or job-tier changes can silently diverge.🤖 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 - 278, The shared query-building logic in QueriesForWithCapability duplicates QueriesFor and can diverge. Extract the common construction, including base filters and JobTier query generation, into a private helper with an optional capability parameter; update both exported functions to be thin wrappers that invoke it, preserving the capability filter behavior when provided.tools/codegen/cmd/featuregate-test-analyzer.go (3)
430-488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated run-count/pass-rate validation logic between the generic loop and the Install-specific block.
The general per-test checks (436-455) and the new Install-specific checks (468-486) implement nearly identical "TotalRuns < required → error" and "passPercent < required → error" patterns, differing only in the threshold and message. Consider extracting a small helper (
validateTestThresholds(testResults, requiredRuns, requiredPassRate, isOptional, ...)) shared by both call sites to avoid the two copies drifting apart as thresholds evolve.🤖 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 430 - 488, The per-test run-count and pass-rate checks are duplicated between the generic TestResults loop and the Install-specific validation. Extract the shared logic into a helper such as validateTestThresholds, parameterized by the test result, required runs, pass rate, optionality, and identifying context, then invoke it from both locations while preserving their distinct thresholds and messages.
961-990: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThread the release version through the per-variant helpers
tools/codegen/cmd/featuregate-test-analyzer.go:825-990—Run()already fetchesrelease, butlistTestResultForVariantandgetInstallTestLevelDataeach callgetRelease()again inside the variant loop. Pass the existing value down throughlistTestResultForso main-branch runs don’t repeat the release lookup for every variant/Install 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 961 - 990, Pass the release value fetched by Run() through listTestResultFor and its per-variant helpers, including getInstallTestLevelData, and remove their internal getRelease() calls. Update signatures and call sites consistently so each variant reuses the existing release while preserving current error handling.
457-488: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse one install aggregation path for both display and enforcement.
Run()reportsgetInstallTestLevelData, while install promotion is decided throughverifyJobBasedFeatureGatePromotion/verifyJobPassRate; if these diverge, the shown percentage can disagree with the gate. Share the aggregation or document the difference.🤖 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 457 - 488, The Install feature-gate validation around verifyJobBasedFeatureGatePromotion must reuse the same aggregation produced by getInstallTestLevelData and used by Run() for display. Update the install pass-rate enforcement, including verifyJobPassRate, to consume that shared result so displayed and enforced percentages cannot diverge; avoid maintaining separate aggregation logic.
🤖 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 213-227: Make the “Install test statistics” iteration
deterministic by collecting the keys from testingResults, sorting them, and
iterating over the sorted job variants instead of ranging over the map directly.
Preserve the existing missing-test, percentage, and zero-run output behavior in
this loop.
---
Duplicate comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 966-981: Restore certificate verification in the Sippy HTTP client
construction used by getInstallTestLevelData by removing the InsecureSkipVerify
override or setting it to false. Also consolidate the duplicated
defaultTransport and sippyClient setup shared with listTestResultForVariant into
a newSippyClient helper, ensuring both callers use the secure configuration.
- Around line 1005-1013: Update the Sippy request construction in
getInstallTestLevelData to use http.NewRequestWithContext with the ctx received
from Run, preserving the existing GET URL and error handling so Run cancellation
stops in-flight fetches.
- Around line 1010-1021: Update the response handling in the sippy request flow
around sippyClient.Do(req) to defer response.Body.Close() immediately after a
successful response is received, ensuring it runs for both non-2xx and read
failures. Preserve the existing status and read error returns, but handle and
propagate the close error rather than discarding it.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 430-488: The per-test run-count and pass-rate checks are
duplicated between the generic TestResults loop and the Install-specific
validation. Extract the shared logic into a helper such as
validateTestThresholds, parameterized by the test result, required runs, pass
rate, optionality, and identifying context, then invoke it from both locations
while preserving their distinct thresholds and messages.
- Around line 961-990: Pass the release value fetched by Run() through
listTestResultFor and its per-variant helpers, including
getInstallTestLevelData, and remove their internal getRelease() calls. Update
signatures and call sites consistently so each variant reuses the existing
release while preserving current error handling.
- Around line 457-488: The Install feature-gate validation around
verifyJobBasedFeatureGatePromotion must reuse the same aggregation produced by
getInstallTestLevelData and used by Run() for display. Update the install
pass-rate enforcement, including verifyJobPassRate, to consume that shared
result so displayed and enforced percentages cannot diverge; avoid maintaining
separate aggregation logic.
In `@tools/codegen/pkg/sippy/json_types.go`:
- Around line 182-278: The shared query-building logic in
QueriesForWithCapability duplicates QueriesFor and can diverge. Extract the
common construction, including base filters and JobTier query generation, into a
private helper with an optional capability parameter; update both exported
functions to be thin wrappers that invoke it, preserving the capability filter
behavior when provided.
🪄 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: 99550354-cc97-48fa-bf30-bd8a2f0787a8
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/codegen/cmd/featuregate-test-analyzer_test.go
| for jobVariant := range testingResults { | ||
| installTest := installTestLevelData[jobVariant] | ||
| if installTest == nil { | ||
| md.Textf(" - %v: \"install should succeed: overall\" test not found\n", jobVariant) | ||
| fmt.Fprintf(o.Out, " %v: \"install should succeed: overall\" test not found\n", jobVariant) | ||
| } else if installTest.TotalRuns > 0 { | ||
| passPercent := float32(installTest.SuccessfulRuns) / float32(installTest.TotalRuns) | ||
| displayActual := int(passPercent * 100) | ||
| md.Textf(" - %v: passed %d%% (%d/%d runs)\n", jobVariant, displayActual, installTest.SuccessfulRuns, installTest.TotalRuns) | ||
| fmt.Fprintf(o.Out, " %v: passed %d%% (%d/%d runs)\n", jobVariant, displayActual, installTest.SuccessfulRuns, installTest.TotalRuns) | ||
| } else { | ||
| md.Textf(" - %v: 0 runs\n", jobVariant) | ||
| fmt.Fprintf(o.Out, " %v: 0 runs\n", jobVariant) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Iteration over testingResults map is nondeterministic.
for jobVariant := range testingResults iterates a Go map in random order, so the "Install test statistics" section's row order will vary between runs of the same data. Other loops in this file (e.g. testNames.List() at line 1168) sort keys before iterating for deterministic output. Since this feeds a generated markdown/HTML artifact, non-deterministic ordering can cause spurious diffs between otherwise-identical runs.
🔧 Proposed fix
- for jobVariant := range testingResults {
+ jobVariants := make([]JobVariant, 0, len(testingResults))
+ for jobVariant := range testingResults {
+ jobVariants = append(jobVariants, jobVariant)
+ }
+ sort.Slice(jobVariants, func(i, j int) bool {
+ return jobVariants[i].String() < jobVariants[j].String()
+ })
+ for _, jobVariant := range jobVariants {📝 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.
| for jobVariant := range testingResults { | |
| installTest := installTestLevelData[jobVariant] | |
| if installTest == nil { | |
| md.Textf(" - %v: \"install should succeed: overall\" test not found\n", jobVariant) | |
| fmt.Fprintf(o.Out, " %v: \"install should succeed: overall\" test not found\n", jobVariant) | |
| } else if installTest.TotalRuns > 0 { | |
| passPercent := float32(installTest.SuccessfulRuns) / float32(installTest.TotalRuns) | |
| displayActual := int(passPercent * 100) | |
| md.Textf(" - %v: passed %d%% (%d/%d runs)\n", jobVariant, displayActual, installTest.SuccessfulRuns, installTest.TotalRuns) | |
| fmt.Fprintf(o.Out, " %v: passed %d%% (%d/%d runs)\n", jobVariant, displayActual, installTest.SuccessfulRuns, installTest.TotalRuns) | |
| } else { | |
| md.Textf(" - %v: 0 runs\n", jobVariant) | |
| fmt.Fprintf(o.Out, " %v: 0 runs\n", jobVariant) | |
| } | |
| } | |
| jobVariants := make([]JobVariant, 0, len(testingResults)) | |
| for jobVariant := range testingResults { | |
| jobVariants = append(jobVariants, jobVariant) | |
| } | |
| sort.Slice(jobVariants, func(i, j int) bool { | |
| return jobVariants[i].String() < jobVariants[j].String() | |
| }) | |
| for _, jobVariant := range jobVariants { | |
| installTest := installTestLevelData[jobVariant] | |
| if installTest == nil { | |
| md.Textf(" - %v: \"install should succeed: overall\" test not found\n", jobVariant) | |
| fmt.Fprintf(o.Out, " %v: \"install should succeed: overall\" test not found\n", jobVariant) | |
| } else if installTest.TotalRuns > 0 { | |
| passPercent := float32(installTest.SuccessfulRuns) / float32(installTest.TotalRuns) | |
| displayActual := int(passPercent * 100) | |
| md.Textf(" - %v: passed %d%% (%d/%d runs)\n", jobVariant, displayActual, installTest.SuccessfulRuns, installTest.TotalRuns) | |
| fmt.Fprintf(o.Out, " %v: passed %d%% (%d/%d runs)\n", jobVariant, displayActual, installTest.SuccessfulRuns, installTest.TotalRuns) | |
| } else { | |
| md.Textf(" - %v: 0 runs\n", jobVariant) | |
| fmt.Fprintf(o.Out, " %v: 0 runs\n", jobVariant) | |
| } | |
| } |
🤖 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 213 - 227, Make
the “Install test statistics” iteration deterministic by collecting the keys
from testingResults, sorting them, and iterating over the sorted job variants
instead of ranging over the map directly. Preserve the existing missing-test,
percentage, and zero-run output behavior in this loop.
|
@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. |
|
Looking at the example, the table that we generate, and the logs are still passing/failing based on the count of periodics and the pass rates of those jobs overall, rather than taking into account the install should succeed rate. I think we want to adjust those outputs in the install case to take into account only the install should succeed rate right? |
3 participants
Augment
verify-feature-promotionoutput to indicate pass percentage forinstall should succeedtests 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.