tooling: Update verify-feature-promotion logic for installer-focused features#2903
Open
everettraven wants to merge 2 commits into
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
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 PR marks AWSDualStackInstall as enabled in feature metadata, feature-gate definitions, and payload manifests. It removes self-managed-high-availability include annotations from several CRDs. It also updates Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/test verify-feature-promotion |
Contributor
|
Hello @everettraven! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/L
Contributor
|
[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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 1092-1104: The hardcoded stub in getJobsForFeatureGateFromSippy is
bypassing the real Sippy lookup and ignoring the release, featureGate, and
variant inputs. Remove the temporary out slice and early return so the function
proceeds to the actual query logic, and keep the existing
getJobsForFeatureGateFromSippy path using the provided parameters to fetch the
correct jobs.
- Around line 1046-1078: The triage handling in verifyJobPassRate is too coarse
and is using all job runs in the pass-rate calculation, which causes triaged
failures to suppress unrelated variants and still count against the denominator.
Update the flow around getTriagedTestFailuresFromSippy and verifyJobPassRate so
triage is keyed by the full variant identity returned by Sippy, not just test
name, and filter out triaged failures before setting
TestResults.TotalRuns/SuccessfulRuns/FailedRuns. Make sure the loop over jobRuns
only increments failures for non-triaged failing runs and that pass-rate math
reflects the filtered set consistently in both affected call sites.
- Around line 1051-1052: The triage lookup result from
getTriagedTestFailuresFromSippy in featuregate-test-analyzer.go is using the
returned err without handling it, which can make failures look untriaged. Update
the logic around triagedTestFailures, err to check and handle the error
immediately: propagate it or stop processing rather than continuing with an
empty triaged set. Keep the fix localized to the code path that consumes
getTriagedTestFailuresFromSippy so the triage counting logic only runs when the
lookup succeeds.
- Around line 999-1008: The defaultTransport setup in featuregate-test-analyzer
should not disable TLS verification for Sippy requests. Remove the
InsecureSkipVerify setting from the tls.Config used by defaultTransport, and
rely on normal certificate validation so the HTTP client still enforces HTTPS
trust checks while preserving the existing transport configuration.
- Around line 993-1016: `verifyInstallFeatureGatePromotion` and the Sippy helper
chain are not propagating cancellation, so requests can outlive `Run(ctx)`.
Update `verifyInstallFeatureGatePromotion` to pass `ctx` into
`getJobsForFeatureGateFromSippy`, then thread it through `getJobRunsFromSippy`
and `getTriagedTestFailuresFromSippy`, and use it when building the HTTP
requests instead of calling `client.Get(...)` directly. Keep the existing
symbols in place, only add context plumbing so request lifetimes follow the
caller’s deadline/cancelation.
🪄 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: 382c96e4-2e33-4b88-b0fe-749c605219d5
📒 Files selected for processing (7)
features.mdfeatures/features.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamltools/codegen/cmd/featuregate-test-analyzer.go
Comment on lines
+999
to
+1008
| 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, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tools/codegen/cmd/featuregate-test-analyzer.go"
echo "== line count =="
wc -l "$file"
echo "== around reported lines =="
sed -n '960,1035p' "$file"
echo "== later helper context =="
sed -n '1035,1125p' "$file"Repository: openshift/api
Length of output: 5480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tools/codegen/cmd/featuregate-test-analyzer.go"
echo "== line count =="
wc -l "$file"
echo "== around reported lines =="
sed -n '960,1035p' "$file"
echo "== later helper context =="
sed -n '1035,1125p' "$file"Repository: openshift/api
Length of output: 5480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="tools/codegen/cmd/featuregate-test-analyzer.go"
echo "== line count =="
wc -l "$file"
echo "== around reported lines =="
sed -n '960,1035p' "$file"
echo "== later helper context =="
sed -n '1035,1125p' "$file"Repository: openshift/api
Length of output: 5480
Don't disable TLS verification for Sippy requests.
InsecureSkipVerify: true allows a MITM to tamper with the job/triage data used here, which can change promotion decisions.
Suggested 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,
- },
}📝 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.
Suggested change
| 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, | |
| }, | |
| defaultTransport := &http.Transport{ | |
| Proxy: http.ProxyFromEnvironment, | |
| ForceAttemptHTTP2: true, | |
| MaxIdleConns: 100, | |
| IdleConnTimeout: 90 * time.Second, | |
| TLSHandshakeTimeout: 10 * time.Second, | |
| ExpectContinueTimeout: 1 * time.Second, | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 1005-1007: 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.23.0)
[ERROR] 1006-1008: 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] 1006-1008: 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 999 - 1008, The
defaultTransport setup in featuregate-test-analyzer should not disable TLS
verification for Sippy requests. Remove the InsecureSkipVerify setting from the
tls.Config used by defaultTransport, and rely on normal certificate validation
so the HTTP client still enforces HTTPS trust checks while preserving the
existing transport configuration.
Source: Linters/SAST tools
Contributor
Author
There was a problem hiding this comment.
We already do this everywhere else and this hasn't been a problem.
There was a problem hiding this comment.
@everettraven Fair point on consistency. I agree this isn’t unique to this PR, but I still think the concern is valid: InsecureSkipVerify: true disables certificate validation for data that feeds promotion decisions, so “it hasn’t been a problem yet” is more luck than guarantee.
That said, if you’d rather keep this PR scoped to the installer-logic change, I’m okay deferring this as broader cleanup instead of blocking here. If you want, I can help turn it into a follow-up issue to remove the insecure TLS config across the related Sippy clients.
dgoodwin
reviewed
Jun 25, 2026
| return false | ||
| } | ||
|
|
||
| func verifyInstallFeatureGatePromotion(featureGate string, jobVariant JobVariant) (*TestingResults, error) { |
Contributor
There was a problem hiding this comment.
suggest going generic, there may be other components that have to prove their fg is ready via a job overall result, not tests within the job.
| ColumnField: "variants", | ||
| OperatorValue: "contains", | ||
| Value: fmt.Sprintf("Topology:%s", variant.Topology), | ||
| }, |
Contributor
There was a problem hiding this comment.
Nice! So in a couple days we could make this a query for variant Capability:FGName (assuming we name this correctly to match the fg)
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 (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
1105-1129: 🎯 Functional Correctness | 🟠 MajorCount empty failed-run cases as failures
Failed job runs with no reported
FailedTestNamesstill fall through toSuccessfulRuns++, so installer or early infra failures get counted as passes. Treat those runs as failures unless they’re explicitly known/triaged.🤖 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 1105 - 1129, The job-run counting logic in the loop over jobRuns is treating failed runs with no FailedTestNames as successes because only untriagedTestFailures increments FailedRuns. Update the featuregate test analyzer to count any jobRun with OverallResult == "F" and not KnownFailure as a failed run, even when FailedTestNames is empty, while still preserving the existing triage filtering for reporting untriaged failures in the jobRun/triagedTestFailures path.
🤖 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 1105-1129: The job-run counting logic in the loop over jobRuns is
treating failed runs with no FailedTestNames as successes because only
untriagedTestFailures increments FailedRuns. Update the featuregate test
analyzer to count any jobRun with OverallResult == "F" and not KnownFailure as a
failed run, even when FailedTestNames is empty, while still preserving the
existing triage filtering for reporting untriaged failures in the
jobRun/triagedTestFailures path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 39488995-d772-40a5-9c03-1230580f496e
📒 Files selected for processing (2)
tools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/pkg/sippy/json_types.go
openshift-ci
Bot
added
the
needs-rebase
everettraven
force-pushed
the
tooling/installer-feature-gate-promotion-updates
branch
from
dbd3ddd to
7b54774
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/codegen/cmd/featuregate-test-analyzer.go (2)
1100-1100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
%wto wrap the error.%qquotes the error string and breakserrors.Is/Aschaining; the sibling helpers all use%w.♻️ Proposed change
- return nil, fmt.Errorf("getting triaged test failures from sippy: %q", err) + return nil, fmt.Errorf("getting triaged test failures from sippy: %w", err)🤖 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` at line 1100, The error handling in the triaged test failures path should wrap the underlying error instead of quoting it. Update the fmt.Errorf call in the helper that fetches triaged test failures from sippy to use %w so errors.Is/errors.As continue to work, matching the pattern used by the sibling helper functions in featuregate-test-analyzer.go.
1043-1067: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse existing constants instead of hardcoding
14/95.requiredNumberOfTestRunsPerVariant = 14(andrequiredPassRateOfTestsPerVariant = 0.95) already exist; hardcoding the same thresholds here risks them drifting out of sync. Considerruns < requiredNumberOfTestRunsPerVariantand deriving the pass-rate check from the existing constant.🤖 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 1043 - 1067, The promotion-threshold checks in the test results logic are hardcoding the same limits already defined elsewhere, so update the logic in the feature gate test analyzer to use the existing constants instead of literal values. In the code around the early-return path and pass-rate gate, reference requiredNumberOfTestRunsPerVariant and derive the 95% check from requiredPassRateOfTestsPerVariant so the thresholds stay consistent.
🤖 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 1139-1141: The error formatting uses the wrong verb for
resp.StatusCode, which is an int, so update the fmt.Errorf messages to use %d
instead of %s at this check and the other two matching status-code checks in
featuregate-test-analyzer.go. Make the same change wherever the code compares
resp.StatusCode against http.StatusOK so the returned error string is correct
and passes go vet.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Line 1100: The error handling in the triaged test failures path should wrap
the underlying error instead of quoting it. Update the fmt.Errorf call in the
helper that fetches triaged test failures from sippy to use %w so
errors.Is/errors.As continue to work, matching the pattern used by the sibling
helper functions in featuregate-test-analyzer.go.
- Around line 1043-1067: The promotion-threshold checks in the test results
logic are hardcoding the same limits already defined elsewhere, so update the
logic in the feature gate test analyzer to use the existing constants instead of
literal values. In the code around the early-return path and pass-rate gate,
reference requiredNumberOfTestRunsPerVariant and derive the 95% check from
requiredPassRateOfTestsPerVariant so the thresholds stay consistent.
🪄 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: cdadfacc-d083-4ec7-82ab-69094b8ab2b3
📒 Files selected for processing (8)
features.mdfeatures/features.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamltools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/pkg/sippy/json_types.go
✅ Files skipped from review due to trivial changes (2)
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- features.md
🚧 Files skipped from review as they are similar to previous changes (5)
- features/features.go
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- tools/codegen/pkg/sippy/json_types.go
Comment on lines
+1139
to
+1141
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("expected a 200 OK status code but got %s", resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrong format verb: %s with an int status code.
resp.StatusCode is an int, so %s renders as %!s(int=404) instead of the actual code, and go vet (run by make lint) will flag this. Use %d. Same issue at Lines 1165-1167 and 1196-1198.
🐛 Proposed fix (apply to all three sites)
- return nil, fmt.Errorf("expected a 200 OK status code but got %s", resp.StatusCode)
+ return nil, fmt.Errorf("expected a 200 OK status code but got %d", resp.StatusCode)📝 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.
Suggested change
| if resp.StatusCode != http.StatusOK { | |
| return nil, fmt.Errorf("expected a 200 OK status code but got %s", resp.StatusCode) | |
| } | |
| if resp.StatusCode != http.StatusOK { | |
| return nil, fmt.Errorf("expected a 200 OK status code but got %d", resp.StatusCode) | |
| } |
🤖 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 1139 - 1141, The
error formatting uses the wrong verb for resp.StatusCode, which is an int, so
update the fmt.Errorf messages to use %d instead of %s at this check and the
other two matching status-code checks in featuregate-test-analyzer.go. Make the
same change wherever the code compares resp.StatusCode against http.StatusOK so
the returned error string is correct and passes go vet.
7b54774 to
35352b3
Compare
openshift-ci
Bot
added
size/XXL
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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.go`:
- Around line 1039-1068: The promotion gate in featuregate-test-analyzer’s
early-return logic is using the wrong run-count threshold; update the checks in
the job analysis flow to use the 14-run promotion requirement instead of
`requiredNumberOfTests` (5). In the `AnalyzeJob`/test result calculation path
where `runs` and `passes` are compared, ensure both the “insufficient results”
branch and the 95% pass-rate early exit are keyed off the 14-run minimum so
installer jobs can’t promote on too little history. Keep the existing pass-rate
behavior unchanged, but align the count guard with the promotion comments and
logic.
- Around line 1135-1149: Move the resp.Body.Close defer to immediately after the
successful client.Get in the affected helpers so every return path closes the
Sippy response body. In the functions that fetch job info and the other two
matching helpers, place defer resp.Body.Close() before any status-code check or
io.ReadAll call, then keep the existing error handling unchanged.
🪄 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: 5011f437-de39-4e5e-bd2a-b2e7140fb99d
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*
📒 Files selected for processing (16)
features.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-OKD.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamltools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/pkg/sippy/json_types.go
💤 Files with no reviewable changes (4)
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-OKD.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-OKD.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-Default.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-Default.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- features.md
🚧 Files skipped from review as they are similar to previous changes (6)
- features/features.go
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- tools/codegen/pkg/sippy/json_types.go
Comment on lines
+1135
to
+1149
| resp, err := client.Get(sippy.BuildSippyJobsForFeatureGateURL(featureGate, release, variant.Topology, variant.Cloud, variant.Architecture, variant.NetworkStack, variant.OS)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting job info: %w", err) | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("expected a 200 OK status code but got %s", resp.StatusCode) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("reading response body: %w", err) | ||
| } | ||
|
|
||
| defer resp.Body.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and map the relevant helpers.
git ls-files tools/codegen/cmd/featuregate-test-analyzer.go
wc -l tools/codegen/cmd/featuregate-test-analyzer.go
ast-grep outline tools/codegen/cmd/featuregate-test-analyzer.go --view expanded || true
# Show the relevant line ranges around the cited code and the other helpers.
sed -n '1110,1215p' tools/codegen/cmd/featuregate-test-analyzer.goRepository: openshift/api
Length of output: 7159
Close the Sippy response body before any early return.
defer resp.Body.Close() needs to run immediately after a successful client.Get; placing it after io.ReadAll leaves non-200 and read-error paths unclosed. Apply the same move in all three helpers.
🤖 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 1135 - 1149,
Move the resp.Body.Close defer to immediately after the successful client.Get in
the affected helpers so every return path closes the Sippy response body. In the
functions that fetch job info and the other two matching helpers, place defer
resp.Body.Close() before any status-code check or io.ReadAll call, then keep the
existing error handling unchanged.
8914d71 to
9945916
Compare
everettraven
changed the title
verify-feature-promotion logic for installer-focused featuresverify-feature-promotion logic for installer-focused features
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
Holding on dropping the temporary promotion commit. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1187-1190: The URL parse error handling in the sippy triages setup
is using fmt.Sprintf with %w and panicking, which should be replaced. In the
code around the reqURL/url.Parse logic, return the wrapped error directly from
the function instead of panic, and use proper error wrapping so the caller
receives the parse failure from this path.
🪄 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: 942d9c4d-1489-44e6-8a48-74d08c681735
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*
📒 Files selected for processing (16)
features.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-OKD.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamltools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/pkg/sippy/json_types.go
💤 Files with no reviewable changes (4)
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-OKD.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-Default.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-Default.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-OKD.crd.yaml
✅ Files skipped from review due to trivial changes (2)
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- features.md
🚧 Files skipped from review as they are similar to previous changes (5)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- features/features.go
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- tools/codegen/pkg/sippy/json_types.go
Comment on lines
+1187
to
+1190
| reqURL, err := url.Parse("https://sippy.dptools.openshift.org/api/component_readiness/triages") | ||
| if err != nil { | ||
| panic(fmt.Sprintf("couldn't parse sippy triages url: %w", err)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
%w is not supported by fmt.Sprintf — and prefer returning the error over panic.
fmt.Sprintf doesn't honor the %w wrapping verb, so this renders as %!w(...) and go vet (via make lint) will fail. Since the function already returns error, return the wrapped error instead of panicking.
🐛 Proposed fix
reqURL, err := url.Parse("https://sippy.dptools.openshift.org/api/component_readiness/triages")
if err != nil {
- panic(fmt.Sprintf("couldn't parse sippy triages url: %w", err))
+ return nil, fmt.Errorf("couldn't parse sippy triages url: %w", err)
}📝 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.
Suggested change
| reqURL, err := url.Parse("https://sippy.dptools.openshift.org/api/component_readiness/triages") | |
| if err != nil { | |
| panic(fmt.Sprintf("couldn't parse sippy triages url: %w", err)) | |
| } | |
| reqURL, err := url.Parse("https://sippy.dptools.openshift.org/api/component_readiness/triages") | |
| if err != nil { | |
| return nil, fmt.Errorf("couldn't parse sippy triages url: %w", err) | |
| } |
🤖 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 1187 - 1190, The
URL parse error handling in the sippy triages setup is using fmt.Sprintf with %w
and panicking, which should be replaced. In the code around the reqURL/url.Parse
logic, return the wrapped error directly from the function instead of panic, and
use proper error wrapping so the caller receives the parse failure from this
path.
so that they can be gated on jobs that install a cluster in the new state that is passing conformance consistently. Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
tooling/installer-feature-gate-promotion-updates
branch
from
9945916 to
e80c0a3
Compare
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
tooling/installer-feature-gate-promotion-updates
branch
from
e80c0a3 to
11e45e9
Compare
openshift-ci
Bot
added
size/XL
Contributor
|
@everettraven: The following tests 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.