Revert "TRT-2817: Revert "Merge pull request #2279 from nbottari9/1814-duplicate-warning""#2322
Revert "TRT-2817: Revert "Merge pull request #2279 from nbottari9/1814-duplicate-warning""#2322hongkailiu wants to merge 1 commit into
Conversation
…tari9/1814-duplicate-warning""
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@hongkailiu: This pull request references TRT-2817 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThe upgrade recommendation flow now detects when CVO evaluates update risks, skips duplicate client-side alert checks when appropriate, loads FeatureGate and Infrastructure mock resources, and adds fixtures and expected outputs for CVO-handled and client-handled risk scenarios. ChangesCVO alert handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant AlertEvaluation
participant FeatureGate
participant Infrastructure
participant ClusterVersion
Command->>AlertEvaluation: evaluate alert ownership
AlertEvaluation->>FeatureGate: read ClusterUpdateAcceptRisks
AlertEvaluation->>Infrastructure: read cluster topology
AlertEvaluation->>ClusterVersion: read desired version
AlertEvaluation-->>Command: return skip or continue
Possibly related PRs
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. 🔧 Checkov (3.3.8)pkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-alerts.jsonTraceback (most recent call last): pkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-cv.yamlTraceback (most recent call last): pkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-featuregate.yamlTraceback (most recent call last):
Comment |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 #2320 |
|
@hongkailiu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/160b6960-8623-11f1-80a8-fc7febfac843-0 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/cli/admin/upgrade/recommend/alerts.go (1)
311-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required function comments.
pkg/cli/admin/upgrade/recommend/alerts.go#L311-L313: document thatisHostedClusteridentifies external control-plane topology.pkg/cli/admin/upgrade/recommend/alerts_test.go#L10-L10: document the feature-gate behavior under test.pkg/cli/admin/upgrade/recommend/alerts_test.go#L97-L97: document the hosted-topology behavior under test.As per coding guidelines, “Add descriptive comments to all exported and unexported Go types, functions, and methods.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/admin/upgrade/recommend/alerts.go` around lines 311 - 313, Add a descriptive comment above isHostedCluster stating that it identifies external control-plane topology. In pkg/cli/admin/upgrade/recommend/alerts_test.go at line 10, document the feature-gate behavior covered by the test; at line 97, document the hosted-topology behavior covered by the test.Source: Coding guidelines
pkg/cli/admin/upgrade/recommend/mockresources.go (1)
85-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap fixture read errors with resource context.
Return errors identifying whether the failed file is the FeatureGate or Infrastructure fixture.
Proposed fix
- return err + return fmt.Errorf("read FeatureGate fixture %q: %w", o.featureGatePath, err) ... - return err + return fmt.Errorf("read Infrastructure fixture %q: %w", o.infrastructurePath, err)Also applies to: 103-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/admin/upgrade/recommend/mockresources.go` around lines 85 - 88, Update the fixture file reads in the resource-loading flow, including the FeatureGate and Infrastructure paths, to wrap non-NotExist errors with context identifying which fixture failed. Preserve the existing handling of missing files and return the contextualized error from the relevant os.ReadFile call.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cli/admin/upgrade/recommend/alerts.go`:
- Around line 26-29: Correct the spelling in the warning message within the
alertsEvaluatedByCVO call handling, changing “occured” to “occurred” without
altering the surrounding control flow or logging behavior.
In
`@pkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.version-5.0.0-ec.3-output`:
- Line 10: Remove the stale ConditionalUpdateRisk error stanza from the
CVO-handled upgrade output fixture, leaving the expected output without a
client-side acceptance failure so it detects regressions to client-side risk
enforcement.
In `@pkg/cli/admin/upgrade/recommend/recommend.go`:
- Around line 341-347: The alertsEvaluatedByCVO lookup error is checked only
inside the true branch, so false-with-error results produce the wrong fallback
message. In the upgrade recommendation flow, check err immediately after calling
alertsEvaluatedByCVO and return a wrapped error using %w before branching on
cvoChecking; preserve the existing messages for successful true and false
results.
- Around line 114-115: Update the fixture-path setup around cvPath to remove the
trailing cvSuffix and append “-featuregate.yaml” and “-infrastructure.yaml” to
that filename stem, avoiding replacement of matching directory segments. In the
alertsEvaluatedByCVO flow, handle and return the lookup error immediately before
branching on cvoChecking, so failures are not hidden by the boolean condition.
---
Nitpick comments:
In `@pkg/cli/admin/upgrade/recommend/alerts.go`:
- Around line 311-313: Add a descriptive comment above isHostedCluster stating
that it identifies external control-plane topology. In
pkg/cli/admin/upgrade/recommend/alerts_test.go at line 10, document the
feature-gate behavior covered by the test; at line 97, document the
hosted-topology behavior covered by the test.
In `@pkg/cli/admin/upgrade/recommend/mockresources.go`:
- Around line 85-88: Update the fixture file reads in the resource-loading flow,
including the FeatureGate and Infrastructure paths, to wrap non-NotExist errors
with context identifying which fixture failed. Preserve the existing handling of
missing files and return the contextualized error from the relevant os.ReadFile
call.
🪄 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: 92cf37e0-1e14-432d-8379-e4fb6b60af2a
📒 Files selected for processing (19)
pkg/cli/admin/upgrade/recommend/alerts.gopkg/cli/admin/upgrade/recommend/alerts_test.gopkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-cv.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-featuregate.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-infrastructure.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.version-5.0.0-ec.3-outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-cv.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-featuregate.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-infrastructure.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks.outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks.version-5.0.0-ec.3-outputpkg/cli/admin/upgrade/recommend/examples_test.gopkg/cli/admin/upgrade/recommend/mockresources.gopkg/cli/admin/upgrade/recommend/recommend.go
| if skip, err := o.alertsEvaluatedByCVO(ctx); err != nil { | ||
| klog.Warningf("An error occured while determining if the CVO is evaluating alerts, so the client will check. %v", err) | ||
| } else if skip { | ||
| return nil, nil |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the warning typo.
“occured” should be “occurred.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cli/admin/upgrade/recommend/alerts.go` around lines 26 - 29, Correct the
spelling in the warning message within the alertsEvaluatedByCVO call handling,
changing “occured” to “occurred” without altering the surrounding control flow
or logging behavior.
| Reason: TestAlert | ||
| Message: Test alert for updates. https://github.com/openshift/runbooks/tree/master/alerts?runbook=notfound | ||
|
|
||
| error: There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: ConditionalUpdateRisk |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the stale client-side acceptance failure.
Line 10 makes the CVO-handled scenario assert the ConditionalUpdateRisk rejection that this flow is intended to bypass. Remove the error stanza so this fixture detects a regression back to client-side risk enforcement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@pkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.version-5.0.0-ec.3-output`
at line 10, Remove the stale ConditionalUpdateRisk error stanza from the
CVO-handled upgrade output fixture, leaving the expected output without a
client-side acceptance failure so it detects regressions to client-side risk
enforcement.
| o.mockData.featureGatePath = strings.Replace(o.mockData.cvPath, cvSuffix, "-featuregate.yaml", 1) | ||
| o.mockData.infrastructurePath = strings.Replace(o.mockData.cvPath, cvSuffix, "-infrastructure.yaml", 1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 8070
🏁 Script executed:
grep -nR "cvPath\|featureGatePath\|infrastructurePath\|alertsEvaluatedByCVO" -n pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 718
🏁 Script executed:
sed -n '320,380p' pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 2974
🏁 Script executed:
sed -n '320,420p' pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 4641
🏁 Script executed:
sed -n '420,520p' pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 3305
🏁 Script executed:
grep -nn "alertsEvaluatedByCVO\|func (o .*alertsEvaluatedByCVO" -n pkg/cli/admin/upgrade/recommend/recommend.go && sed -n '380,460p' pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 2992
🏁 Script executed:
sed -n '330,460p' pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 5294
🏁 Script executed:
grep -n "func (o .*alertsEvaluatedByCVO\|alertsEvaluatedByCVO(ctx" -n pkg/cli/admin/upgrade/recommend/recommend.go && sed -n '460,560p' pkg/cli/admin/upgrade/recommend/recommend.goRepository: openshift/oc
Length of output: 3449
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('pkg/cli/admin/upgrade/recommend/recommend.go')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'alertsEvaluatedByCVO' in line:
print(i, line)
PYRepository: openshift/oc
Length of output: 226
🏁 Script executed:
rg -n "alertsEvaluatedByCVO" pkg/cli/admin/upgrade/recommendRepository: openshift/oc
Length of output: 640
🏁 Script executed:
rg -n "func .*alertsEvaluatedByCVO|alertsEvaluatedByCVO\\(" pkg/cli/admin/upgrade/recommendRepository: openshift/oc
Length of output: 497
🏁 Script executed:
sed -n '240,320p' pkg/cli/admin/upgrade/recommend/alerts.goRepository: openshift/oc
Length of output: 2510
🏁 Script executed:
sed -n '1,120p' pkg/cli/admin/upgrade/recommend/alerts.goRepository: openshift/oc
Length of output: 3785
🏁 Script executed:
sed -n '240,340p' pkg/cli/admin/upgrade/recommend/alerts.goRepository: openshift/oc
Length of output: 2510
Derive the companion fixture paths from the filename suffix, and surface CVO lookup errors first. strings.Replace(..., 1) can rewrite a matching directory segment in cvPath, and alertsEvaluatedByCVO returns false, err on failure, so the nested err != nil check under cvoChecking never runs. Trim the trailing suffix before appending the companion names, and handle err before branching on cvoChecking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cli/admin/upgrade/recommend/recommend.go` around lines 114 - 115, Update
the fixture-path setup around cvPath to remove the trailing cvSuffix and append
“-featuregate.yaml” and “-infrastructure.yaml” to that filename stem, avoiding
replacement of matching directory segments. In the alertsEvaluatedByCVO flow,
handle and return the lookup error immediately before branching on cvoChecking,
so failures are not hidden by the boolean condition.
| if cvoChecking, err := o.alertsEvaluatedByCVO(ctx); cvoChecking { | ||
| if err != nil { | ||
| return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err) | ||
| } | ||
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ",")) | ||
| } else { | ||
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle the lookup error before branching.
alertsEvaluatedByCVO returns false, err, so Lines 342-344 are unreachable and the error falls through to the generic --accept message. Check err first and wrap it with %w, or explicitly adopt the same warning-and-fallback policy as alerts().
Proposed fix
- if cvoChecking, err := o.alertsEvaluatedByCVO(ctx); cvoChecking {
- if err != nil {
- return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err)
- }
+ cvoChecking, err := o.alertsEvaluatedByCVO(ctx)
+ if err != nil {
+ return fmt.Errorf("determining whether CVO checks alerts: %w", err)
+ }
+ if cvoChecking {
return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ","))
- } else {
- return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ","))
}
+ return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ","))As per path instructions, “Never ignore error returns.”
📝 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.
| if cvoChecking, err := o.alertsEvaluatedByCVO(ctx); cvoChecking { | |
| if err != nil { | |
| return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err) | |
| } | |
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ",")) | |
| } else { | |
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) | |
| cvoChecking, err := o.alertsEvaluatedByCVO(ctx) | |
| if err != nil { | |
| return fmt.Errorf("determining whether CVO checks alerts: %w", err) | |
| } | |
| if cvoChecking { | |
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ",")) | |
| } else { | |
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cli/admin/upgrade/recommend/recommend.go` around lines 341 - 347, The
alertsEvaluatedByCVO lookup error is checked only inside the true branch, so
false-with-error results produce the wrong fallback message. In the upgrade
recommendation flow, check err immediately after calling alertsEvaluatedByCVO
and return a wrapped error using %w before branching on cvoChecking; preserve
the existing messages for successful true and false results.
Source: Path instructions
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 openshift/origin#31417 |
|
@hongkailiu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/31da0c00-8682-11f1-9066-6af507414644-0 |
|
@hongkailiu: This PR was included in a payload test run from openshift/origin#31417
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e347f500-869c-11f1-8fbe-6e680a895c06-0 |
|
/pipeline auto |
|
Pipeline controller notification The |
|
Scheduling required tests: |
|
/test e2e-aws-ovn-serial-2of2 |
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 Expecting a failure. but after openshift/origin#31417 gets in, expecting a success. |
|
@hongkailiu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0b9dd0c0-86c4-11f1-8028-419d584ec89f-0 |
|
@hongkailiu: 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. |
Reverts #2320
/hold
requires openshift/origin#31417 to go first.
Summary by CodeRabbit
New Features
Bug Fixes
Tests