sql: fix DataCacheConfig diff when enabled=false#7145
Conversation
1c710b8 to
0692efd
Compare
e343283 to
1e68a81
Compare
|
--- INVESTIGATION REPORT --- (This report was generated by Overseer) |
codebot-robot
left a comment
There was a problem hiding this comment.
This is a clean and solid fix that successfully addresses the DataCacheConfig false diff loop when DataCacheEnabled is disabled. The changes precisely mimic GCP's API response behavior by entirely omitting the DataCacheConfig object, and the mock updates align nicely with this real response format. Overall, the fix is logically sound and testing is properly updated.
I have left a few minor maintainability suggestions aimed at improving code conciseness, preserving context about metadata fields (like ForceSendFields) previously documented in the equality function, and future-proofing the equality check if GCP adds more fields to DataCacheConfig down the line.
(This review was generated by Overseer)
| return true | ||
| // GCP omits DataCacheConfig when DataCacheEnabled is false. | ||
| // Treat nil and false as equivalent. | ||
| desiredEnabled := false |
There was a problem hiding this comment.
If this nil-safe getter pattern is used frequently or if this function grows, consider abstracting this into a simple helper like isDataCacheEnabled(config *api.DataCacheConfig) bool. This would reduce the body of this check to return isDataCacheEnabled(desired) == isDataCacheEnabled(actual).
| // Ignore ForceSendFields. Assume it is set correctly in desired. | ||
| // Ignore NullFields. Assume it is set correctly in desired. | ||
| return true | ||
| return desiredEnabled == actualEnabled |
There was a problem hiding this comment.
Returning a straight boolean equivalence here effectively discards checking any other fields in the struct. While DataCacheEnabled is the only substantive property today, any future fields added by GCP (e.g., a cache size setting) will be silently ignored when both sides evaluate as enabled. Consider restructuring this to perform a field-by-field check (or adding a TODO comment) if both caches are enabled.
There was a problem hiding this comment.
Not blocking but I do think that at some point fields may get added. When that happens we would need to fix this code anyway but either a TODO or a slight restructure like the following would make that easier.
if desiredEnabled != actualEnabled {
return false
}
// All fields match returning true
return true
| return true | ||
| // GCP omits DataCacheConfig when DataCacheEnabled is false. | ||
| // Treat nil and false as equivalent. | ||
| desiredEnabled := false |
There was a problem hiding this comment.
This block can be simplified into a single, idiomatic inline assignment to improve conciseness:
|
@justinsb This PR is successful, mergeable and ready for review. Could you please review and approve it? (Overseer) |
1 similar comment
|
@justinsb This PR is successful, mergeable and ready for review. Could you please review and approve it? (Overseer) |
|
Starting task: address-feedback (new feedback since last commit/action) |
codebot-robot
left a comment
There was a problem hiding this comment.
Thanks for the feedback! I've updated the PR:
- Abstracted the
nil-safe getter pattern into anisDataCacheEnabledhelper. - Restructured the boolean equivalence check to explicitly return early for the disabled case, leaving room for future field-by-field comparisons if both are enabled.
- Added a comment in
mocksqlexplaining the GCP response behavior. - Simplified the
desiredEnabled/actualEnabledassignments.
(This comment was generated by Overseer)
|
@maqiuyujoyce This PR is successful, mergeable and ready for review. Could you please review and approve it? (Overseer) |
cheftako
left a comment
There was a problem hiding this comment.
There is a nice to have here but not blocking.
| // Ignore ForceSendFields. Assume it is set correctly in desired. | ||
| // Ignore NullFields. Assume it is set correctly in desired. | ||
| return true | ||
| return desiredEnabled == actualEnabled |
There was a problem hiding this comment.
Not blocking but I do think that at some point fields may get added. When that happens we would need to fix this code anyway but either a TODO or a slight restructure like the following would make that easier.
if desiredEnabled != actualEnabled {
return false
}
// All fields match returning true
return true
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako 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 |
|
/cc @acpana |
a2fb838
cherry picks #7145 onto `release-1.134`
BRIEF Change description
This PR fixes a diff in
SQLInstancewhere.settings.dataCacheConfigis incorrectly detected as different whendataCacheEnabledisfalse, because GCP omits the field in its response.Fixes #7144
WHY do we need this change?
When
dataCacheEnabledis set tofalse, GCP returns anilobject fordataCacheConfig. The previous equality logic usedPointersMatch, which failed when comparing a non-nildesiredobject (withdataCacheEnabled: false) against anilactualobject.Special notes for your reviewer:
None.
Does this PR add something which needs to be 'release noted'?
NONE
Additional documentation e.g., references, usage docs, etc.:
NONE
Intended Milestone
NONE
Tests you have done
hack/compare-mock pkg/test/resourcefixture/testdata/basic/sql/v1beta1/sqlinstance/sqlinstance-datacacheconfig-directthat the resource becomesReady.