fully synchronize CRDs and helm values

[chrisseto]

Prior to this commit a variety of differences between the redpanda CRD and the helm values existed. This could prevent end users from leveraging certain features of the chart/operator and cause strange behaviors in cases of incorrect serialization.

To ensure these differences don't reappear, a new property test (leveraging a fork of rapid) has been added that asserts that the JSON serialization of the CRD and typed helm values are the same. This test ignores a handful of fields which are documented in the test itself.

The resultant heavy modifications to the CRD should be backwards compatible with existing installations. The majority of changes were either additions of optional fields or making required fields optional.

Fixes #146, #81

[chrisseto]

The aspect I'm most worried about is the fullname change. I'm keeping the field around so we don't suddenly break existing deployments. I think the change is safe though as existing usages of that field would either a) do nothing or b) break horribly because helm wouldn't pick it up and some of the internal controllers' logic would break.

Please check my math.

[andrewstucki]

Found one seeming field that looks like it was dropped.

[andrewstucki]

The aspect I'm most worried about is the fullname change. I'm keeping the field around so we don't suddenly break existing deployments. I think the change is safe though as existing usages of that field would either a) do nothing or b) break horribly because helm wouldn't pick it up and some of the internal controllers' logic would break.

Still getting up-to-speed, but let me attempt to parse out the context/worry about this change:

In our helm chart we likely are using fullnameOverride v. fullNameOverride with the camelized Name? Hence in older versions of this CRD passing in a fullNameOverride would either:

A. Do nothing since the code to initialize the helm values is just marshaling the spec from the Redpanda CRD
B. Break things because of the point above coupled with whatever assumptions we might make in controller code about the value of the mis-tagged FullnameOverride field being reflected in reality?

If that's what you're saying, seems reasonable to me to keep the field around and just ignore it.

On the flip side though, is there a chance that we have any controller code that relies on the field essentially being ignored (i.e. always uses some default value) and fixing this might cause the controller code to break?

[chrisseto]

On the flip side though, is there a chance that we have any controller code that relies on the field essentially being ignored (i.e. always uses some default value) and fixing this might cause the controller code to break?

The opposite actually. It seems like fullNameOverride would be respected when doing so would cause controller code to fail. Yet another example of how lack luster the current test coverage is 😬

[andrewstucki]

I'm generally fine with these changes. I do still have a question about how we want to treat the deprecated fullNameOverride field though - should we plan on migrating the field to the new fullnameOverride when initializing the chart values to fix anyone that might be broken or just ignore it and use the new field? Here it looks like we're just ignoring it and keeping the field only for storage compatibility purposes. Either way is good with me, but just wondering if we had a typical way of handling stuff like this if it's happened before?

Also, if you feel comfortable with the rest of the changes, I'd say go ahead and merge, but if you want a more experienced with this codebase set of eyes first... maybe wait for Rafal to get back?

[chrisseto]

should we plan on migrating the field to the new fullnameOverride when initializing the chart values to fix anyone that might be broken or just ignore it and use the new field?

I think that migrating it would likely end up in bugs being opened as it would trigger a massive and unexpected change to the existing deployment. For anything that would change an existing deployment in meaningful ways, I think it's best to make those opt-in. Predictability trumps correctness for something running someone else's software.

Also, if you feel comfortable with the rest of the changes, I'd say go ahead and merge, but if you want a more experienced with this codebase set of eyes first... maybe wait for Rafal to get back?

I'm pretty comfortable with merging this but I will wait until Monday to release it 😅