Allow resources to contain unexported fields #487
Conversation
Using unexported fields in Kubernetes resources is not common, but does happen. Previously, these fields would cause cmp.Diff to panic. Now, we ignore the content of unexported fields when computing a diff. As the diff is for logging and test assertions this is relatively safe. Semantic equality is used for detecting when a managed resource needs to be updated. Custom equality func can be defined separately as needed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
==========================================
+ Coverage 61.03% 61.12% +0.09%
==========================================
Files 27 28 +1
Lines 2556 2562 +6
==========================================
+ Hits 1560 1566 +6
Misses 909 909
Partials 87 87 ☔ View full report in Codecov by Sentry. |
| // +kubebuilder:object:generate=true | ||
| type TestResourceUnexportedFieldsSpec struct { | ||
| Fields map[string]string `json:"fields,omitempty"` | ||
| unexportedFields map[string]string |
There was a problem hiding this comment.
it wasn't clear to me whether the cmp IgnoreUnexported option works recursively or for the top level struct only. IMHO, it'd be useful to have a nested struct with unexported fields as part of this spec.
There was a problem hiding this comment.
I see you have implemented your own opt! How does it handle nested structs with unexported fields?
There was a problem hiding this comment.
Within cmp, each option is applied to every path discovered while walking the object tree. If the path is filtered no fields under it are processed.
There was a problem hiding this comment.
⭐ → @scothis! Thank you very much for the effort. I specifically took note of how you are driving it with TestResourceUnexportedFields.
@neowulf would you be able to test-drive this branch with your kit?
Since ExpectConfig is using IgnoreAllUnexported to verify all CRUD events, there would no more immediate need to open up cmp options for configuration as far as #484 is concerned, or is there? Correct me if I am wrong.
| var IgnoreAllUnexported = cmp.FilterPath(func(p cmp.Path) bool { | ||
| // from cmp.IgnoreUnexported with type info removed | ||
| sf, ok := p.Index(-1).(cmp.StructField) | ||
| if !ok { | ||
| return false | ||
| } | ||
| r, _ := utf8.DecodeRuneInString(sf.Name()) | ||
| return !unicode.IsUpper(r) | ||
| }, cmp.Ignore()) |
There was a problem hiding this comment.
[praise]: This!
I had started looking into it and had planned to put to the test what was suggested by google/go-cmp#313 (comment). Very glad that you were able to bring it here for good effect.
There was a problem hiding this comment.
Funny, we cribbed from the same source to arrive at the identical outcome
| @@ -427,6 +427,7 @@ var ( | |||
| return str != "" && !strings.HasPrefix(str, "Status") | |||
| }, cmp.Ignore()) | |||
|
|
|||
| // Deprecated: use reconcilers.IgnoreAllUnexported instead | |||
There was a problem hiding this comment.
[praise]: Thanks for this! Will schedule removal in next+1 version. https://github.com/search?q=SafeDeployDiff&type=code suggests that it's not used by public projects on Github at least.
|
[question, non-blocking]: @scothis when you say
are you meaning that there could potentially be a follow-up PR? I am asking because afaik there's currently no way to provide a custom equality func, or is there? |
|
@mamachanko I was basing it on this comment #484 (comment) |
|
@scothis this fix works! Thank you again for the quick turnaround! |
|
@neowulf cool, I'll circle back to Max's feedback tomorrow. |
Signed-off-by: Scott Andrews <[email protected]>
|
Thank you @scothis ! |
|
@scothis, VMware has approved your signed contributor license agreement. |
Using unexported fields in Kubernetes resources is not common, but does happen. Previously, these fields would cause cmp.Diff to panic. Now, we ignore the content of unexported fields when computing a diff. As the diff is for logging and test assertions this is relatively safe.
Semantic equality is used for detecting when a managed resource needs to be updated. Custom equality func can be defined separately as needed.
Refs #484