Add custom cmp.Options when comparing k8s objects

[neowulf]

This fix allows adding custom comparator options and is especially useful when non standard k8s objects are used and may require custom comparators.

Resolves https://github.com/vmware-labs/reconciler-runtime/issues/484

[neowulf]

I am unable to assign.

Requesting feedback from @squeedee @mamachanko @scothis

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 61.03%. Comparing base (3180680) to head (f061f6d).

Files	Patch %	Lines
testing/subreconciler.go	0.00%	3 Missing :warning:
testing/reconciler.go	0.00%	1 Missing :warning:
testing/webhook.go	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #486 +/- ## ======================================= Coverage 61.03% 61.03% ======================================= Files 27 27 Lines 2556 2556 ======================================= Hits 1560 1560 Misses 909 909 Partials 87 87 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[scothis]

I generally agree with @mamachanko about avoiding globals. For the non-test code, the diff is only used for log statements. If there is a way to ignore all unexpected fields, we should use that (I'm not sure there is, at least not built-in).

While I think we should open up cmp options to be user configurable, I personally find depending on a third party API to be an anti-pattern. In controllers I've created that use external apis (outside of the built-in k8s types) I'll create a copy of those types that I control that mirrors the API shape. This not only avoids issues with how the types are defined, but also avoids dependency hell by not pulling in the all the dependencies of that API package. The k8s ecosystem has trouble not making breaking changes in point releases that cause the entire ecosystem to be on either one side of that break or the other. The fewer dependencies you have, the easier it is to upgrade past that breaking change.

[neowulf]

Thank you for the feedback. In order to accommodate these types using this fix , I am able to move on by doing the following:

func init() {
    rtesting.CmpOptions = append(rtesting.CmpOptions, protocmp.Transform())
}

and when I need to use SemanticEquals (irrespective of this PR) , I have been doing this:

func init() {
        // this is for DestinationRule only. Need to define for each Istio data type that's in use 
    if err := equality.Semantic.AddFunc(func(dr1, dr2 *v1beta1.DestinationRule) bool {
        return cmp.Equal(&dr1.Spec, &dr2.Spec, protocmp.Transform())
    }); err != nil {
        panic(err)
    }
}

I am happy to provide test cases and additional documentation to get this MR merged provided this is the fix we want. Which at this point, it seems to me we want to go in a different direction. Please advise.

[neowulf]

Hi folks, requesting feedback for next steps

[mamachanko]

Inspired by @scothis's points, how about the following:

• Try to ignore all unexported fields when diffing for the reconciler logs (@mamachanko)
• Triage if maintaining an copy of upstream istio types works for you (@neowulf)
• Open rtesting's diffing/equality to configuration either by making cmp options user-configurable or a VerifyCreate field in the testsuite or ...

[scothis]

I took a crack at applying a cmp.Option to ignore all unexported fields in #487

This should handle @neowulf's immediate concern. I still think we should open up custom opts, but how to approach that needs a bit more thought.