Fix typos and remove literal quotes in error messages - #276
Conversation
openshift-ci
Bot
added
the
needs-rebase
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughCompare error messages now use corrected quoting and spelling. Matching golden files reflect the updated output. Cluster-side missing inline-diff values remain non-error. ChangesCompare error messages
Estimated code review effort: 2 (Simple) | ~10 minutes 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: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[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 |
c12b2de to
edd237d
Compare
openshift-ci
Bot
removed
the
needs-rebase
edd237d to
c6f72ee
Compare
c6f72ee to
c051d1b
Compare
![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
🤖 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/compare/compare.go`:
- Around line 1070-1075: The inline-diff preprocessing in compare.go is treating
any false found state from NestedString as a missing field before checking the
returned error, which can mask malformed cluster data. Update the logic around
clusterValue, exist, and err so the error from NestedString is handled first and
reported with the existing fmt.Errorf path in the inline-diff preprocessing
block, and only continue when the field is truly absent with no error.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 14e0a7d4-53c5-4e75-9fd6-b1e3330053ee
📒 Files selected for processing (5)
pkg/compare/compare.gopkg/compare/testdata/NoInput/localerr.goldenpkg/compare/testdata/ReferenceConfigFileDoesntExist/localerr.goldenpkg/compare/testdata/ReferenceV2PerFieldMatcherValidation/localpathNotItTemplateerr.goldenpkg/compare/testdata/UserOverride/localfailOutputerr.golden
✅ Files skipped from review due to trivial changes (4)
- pkg/compare/testdata/NoInput/localerr.golden
- pkg/compare/testdata/ReferenceConfigFileDoesntExist/localerr.golden
- pkg/compare/testdata/ReferenceV2PerFieldMatcherValidation/localpathNotItTemplateerr.golden
- pkg/compare/testdata/UserOverride/localfailOutputerr.golden
| clusterValue, exist, err := NestedString(obj.clusterObj.Object, listedPath...) | ||
| if !exist { | ||
| continue // if value does not appear in cluster CR then there will be a diff anyway and this is not an error | ||
| } | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to acces value in cluster cr of field %s that uses inline diff func: %w", pathToKey, err)) | ||
| errs = append(errs, fmt.Errorf("failed to access value in cluster cr of field %s that uses inline diff func: %w", pathToKey, err)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In k8s.io/apimachinery v0.34.0, can unstructured.NestedString return found=false together with a non-nil error when traversal hits a non-map value or the resolved field is not a string?
💡 Result:
In k8s.io/apimachinery v0.34.0, the function unstructured.NestedString does not return found=false together with a non-nil error. Instead, its behavior depends on whether the field is found and whether it is a string: 1. If the field is found and it is a string, it returns the string, found=true, and error=nil [1][2]. 2. If the field is not found (or an intermediate path is missing), it returns an empty string, found=false, and error=nil [1][2]. 3. If the field is found but is not a string, it returns an empty string, found=false, and a non-nil error (specifically an accessor error) [1][2]. The implementation explicitly checks the found status and error from NestedFieldNoCopy before performing a type assertion [1][2]: func NestedString(obj map[string]interface{}, fields...string) (string, bool, error) { val, found, err:= NestedFieldNoCopy(obj, fields...) if!found || err!= nil { return "", found, err } s, ok:= val.(string) if!ok { return "", false, fmt.Errorf("%v accessor error: %v is of the type %T, expected string", jsonPath(fields), val, val) } return s, true, nil } Therefore, a non-nil error is only returned when the field exists but is of an incorrect type, and in that specific case, found is returned as false [1][2]. If the field is missing (found=false), the error is nil [1][2].
Citations:
- 1: https://github.com/kubernetes/apimachinery/blob/v0.35.3/pkg/apis/meta/v1/unstructured/helpers.go
- 2: https://github.com/kubernetes/apimachinery/blob/95b78024e3feada7739b40426690b4f287933fd8/pkg/apis/meta/v1/unstructured/helpers.go
Check the error before treating the cluster value as missing. unstructured.NestedString returns found=false and a non-nil error for type mismatches, so the current if !exist { continue } path can hide malformed cluster data as “missing” instead of surfacing the inline-diff preprocessing error.
Suggested fix
clusterValue, exist, err := NestedString(obj.clusterObj.Object, listedPath...)
- if !exist {
- continue // if value does not appear in cluster CR then there will be a diff anyway and this is not an error
- }
if err != nil {
errs = append(errs, fmt.Errorf("failed to access value in cluster cr of field %s that uses inline diff func: %w", pathToKey, err))
continue
}
+ if !exist {
+ continue // if value does not appear in cluster CR then there will be a diff anyway and this is not an error
+ }📝 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.
| clusterValue, exist, err := NestedString(obj.clusterObj.Object, listedPath...) | |
| if !exist { | |
| continue // if value does not appear in cluster CR then there will be a diff anyway and this is not an error | |
| } | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("failed to acces value in cluster cr of field %s that uses inline diff func: %w", pathToKey, err)) | |
| errs = append(errs, fmt.Errorf("failed to access value in cluster cr of field %s that uses inline diff func: %w", pathToKey, err)) | |
| clusterValue, exist, err := NestedString(obj.clusterObj.Object, listedPath...) | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("failed to access value in cluster cr of field %s that uses inline diff func: %w", pathToKey, err)) | |
| continue | |
| } | |
| if !exist { | |
| continue // if value does not appear in cluster CR then there will be a diff anyway and this is not an error | |
| } |
🤖 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/compare/compare.go` around lines 1070 - 1075, The inline-diff
preprocessing in compare.go is treating any false found state from NestedString
as a missing field before checking the returned error, which can mask malformed
cluster data. Update the logic around clusterValue, exist, and err so the error
from NestedString is handled first and reported with the existing fmt.Errorf
path in the inline-diff preprocessing block, and only continue when the field is
truly absent with no error.
Fix spelling errors in error strings ("occurered" → "occurred",
"acces" → "access", "entires" → "entries") and remove unnecessary
escaped literal quotes wrapping two error constants. Update golden
test files to match.
c051d1b to
3fd8eec
Compare
|
@sebrandon1: 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. |
1 participant
Summary
noRefFileWasPassedandrefFileNotExistsErrorerror constants that were appearing in user-facing outputJira: https://redhat.atlassian.net/browse/CNF-23706
Summary by CodeRabbit