OCPSTRAT-2876: Teach config operator to filter feature gate manifests by major version inclusion#459
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughParses the operator version with semver and adds manifest-level filtering by operator major version during Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/starter.go (1)
393-406: Consider avoiding variable shadowing for clarity.The loop variable
version(string) is shadowed by the parsed variable (uint64) at line 395. This makes the error message at line 398 less helpful, as it prints the parsed number instead of the original string that failed to parse.🔎 Proposed refactor
func includesDesiredVersion(versions []string, operatorVersion uint64) (bool, error) { - for _, version := range versions { - version, err := strconv.ParseUint(version, 10, 64) + for _, versionStr := range versions { + version, err := strconv.ParseUint(versionStr, 10, 64) if err != nil { // Malformed annotation so should be excluded - return false, fmt.Errorf("malformed annotation: %s", version) + return false, fmt.Errorf("malformed annotation: %s", versionStr) } if version == operatorVersion { return true, nil } } return false, nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/operator/starter.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/operator/starter.go
🔇 Additional comments (3)
pkg/operator/starter.go (3)
249-252: LGTM!The operator version parsing uses
ParseTolerant, which is appropriate for handling various version formats. Error handling is correct, and only the major version is extracted for filtering, which aligns with the feature requirements.
284-287: LGTM!The version-based filtering is correctly implemented. Manifests that don't apply to the current operator version are appropriately skipped during the walk.
12-12: Library verification complete:github.com/blang/semver/v4v4.0.0 is the latest stable version with no known security vulnerabilities.The import is safe to use.
JoelSpeed
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@JoelSpeed: This pull request references OCPSTRAT-2876 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 feature to target the "4.22.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. |
pkg/operator/starter.go
Outdated
| return &ret.Status.OperatorStatusApplyConfiguration, nil | ||
| } | ||
|
|
||
| func excludesOperatorVersion(annotations map[string]string, operatorVersion uint64) bool { |
There was a problem hiding this comment.
Shall we use a more specific parameter name? for example operatorMajorVersion instead of operatorVersion
pkg/operator/starter.go
Outdated
| return !hasOperatorVersion | ||
| } | ||
|
|
||
| func includesDesiredVersion(versions []string, operatorVersion uint64) (bool, error) { |
There was a problem hiding this comment.
Shall we use a more specific parameter name? for example operatorMajorVersion instead of operatorVersion
pkg/operator/starter.go
Outdated
|
|
||
| func includesDesiredVersion(versions []string, operatorVersion uint64) (bool, error) { | ||
| for _, version := range versions { | ||
| version, err := strconv.ParseUint(version, 10, 64) |
There was a problem hiding this comment.
Should we consider versions prefixed with "-"? we use err to catch that case?
JoelSpeed
force-pushed
the
major-version-fitlering
branch
from
7e75885 to
af37eda
Compare
|
Thank you for the review @JianLi-RH, I've pushed some changes to make use of more specific variable names, trimming the space and handling of negated versions Do you have time to take another look through? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/starter.go`:
- Around line 419-423: The code shadows the original string variable named
version by assigning ParseUint's result back into version, so on parse failure
the error prints a zero value; change the ParseUint call to store its uint64
result in a new identifier (e.g., parsedVersion) and update the error
construction to include the original string (use %q) or include the parse error;
specifically modify the strconv.ParseUint call and the subsequent fmt.Errorf in
pkg/operator/starter.go so it references parsedVersion for the numeric value and
uses the original version string in the error message to preserve the malformed
input for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0185c357-d6bc-4401-a59c-6794301aef0a
📒 Files selected for processing (1)
pkg/operator/starter.go
pkg/operator/starter.go
Outdated
| version, err := strconv.ParseUint(version, 10, 64) | ||
| if err != nil { | ||
| // Malformed annotation so should be excluded | ||
| return false, fmt.Errorf("malformed annotation: %d", version) | ||
| } |
There was a problem hiding this comment.
Variable shadowing loses the original string in error message.
The version variable is shadowed by the uint64 result of ParseUint. When parsing fails, the error message reports 0 instead of the malformed string, making debugging harder.
🛠️ Proposed fix
- version, err := strconv.ParseUint(version, 10, 64)
+ parsedVersion, err := strconv.ParseUint(version, 10, 64)
if err != nil {
// Malformed annotation so should be excluded
- return false, fmt.Errorf("malformed annotation: %d", version)
+ return false, fmt.Errorf("malformed annotation: %s", version)
}
- if version == desiredVersion {
+ if parsedVersion == desiredVersion {
return true, nil
}📝 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.
| version, err := strconv.ParseUint(version, 10, 64) | |
| if err != nil { | |
| // Malformed annotation so should be excluded | |
| return false, fmt.Errorf("malformed annotation: %d", version) | |
| } | |
| parsedVersion, err := strconv.ParseUint(version, 10, 64) | |
| if err != nil { | |
| // Malformed annotation so should be excluded | |
| return false, fmt.Errorf("malformed annotation: %s", version) | |
| } | |
| if parsedVersion == desiredVersion { | |
| return true, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/starter.go` around lines 419 - 423, The code shadows the
original string variable named version by assigning ParseUint's result back into
version, so on parse failure the error prints a zero value; change the ParseUint
call to store its uint64 result in a new identifier (e.g., parsedVersion) and
update the error construction to include the original string (use %q) or include
the parse error; specifically modify the strconv.ParseUint call and the
subsequent fmt.Errorf in pkg/operator/starter.go so it references parsedVersion
for the numeric value and uses the original version string in the error message
to preserve the malformed input for debugging.
af37eda to
437837a
Compare
JoelSpeed
force-pushed
the
major-version-fitlering
branch
from
437837a to
1548c55
Compare
|
/pipeline required |
|
@JoelSpeed: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, JoelSpeed 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 |
|
/verified by @JianLi-RH |
openshift-ci-robot
added
the
verified
|
@JianLi-RH: This PR has been marked as verified by 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. |
|
Here is my test result: https://redhat.atlassian.net/browse/OCPSTRAT-2876?focusedCommentId=16489513 |
4 participants
This teaches the config operator to filter based on major-version, see openshift/library-go#2069 and openshift/api#2637