feat: Support objectOverrides
#1118
Conversation
Techassi
left a comment
Techassi
left a comment
There was a problem hiding this comment.
Mostly lgtm, a few various question, comments and suggestions.
| ### Added | ||
|
|
||
| - Support `objectOverrides` ([#1118]). |
There was a problem hiding this comment.
note: I think we should add a small explanation how this works/what this does.
| ### Removed | ||
|
|
||
| - BREAKING: `ClusterResources` no longer derives `Eq` and `PartialEq` ([#1118]). |
There was a problem hiding this comment.
question: Whats the reason those two trait implementations are removed?
| let mutated = resource.maybe_mutate(&self.apply_strategy); | ||
| let mut mutated = resource.maybe_mutate(&self.apply_strategy); | ||
|
|
||
| // We apply the object overrides of the user at the very last to offer maximum flexibility. |
There was a problem hiding this comment.
| // We apply the object overrides of the user at the very last to offer maximum flexibility. | |
| // We apply the object overrides of the user at the very end to offer maximum flexibility. |
| merge_strategies::map::granular( | ||
| &mut self.extra_pod_selector_labels, | ||
| other.extra_pod_selector_labels, | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); | ||
| merge_strategies::list::map( | ||
| &mut self.ports, | ||
| other.ports, | ||
| &[|lhs, rhs| lhs.name == rhs.name], | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
note: Please add two dev comments to quickly explain what this code does and why we need this specialized merge.
| merge_strategies::list::map( | ||
| &mut self.ingress_addresses, | ||
| other.ingress_addresses, | ||
| &[|lhs, rhs| lhs.address == rhs.address], | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); | ||
| merge_strategies::map::granular( | ||
| &mut self.node_ports, | ||
| other.node_ports, | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); |
| let Some(patch_type) = &patch.types else { | ||
| return Ok(()); | ||
| }; | ||
| if patch_type.api_version != R::api_version(&()) || patch_type.kind != R::kind(&()) { |
There was a problem hiding this comment.
| if patch_type.api_version != R::api_version(&()) || patch_type.kind != R::kind(&()) { | |
| if object_type.api_version != R::api_version(&()) || object_type.kind != R::kind(&()) { |
| if patch_type.api_version != R::api_version(&()) || patch_type.kind != R::kind(&()) { | ||
| return Ok(()); | ||
| } | ||
| let Some(patch_name) = &patch.metadata.name else { |
There was a problem hiding this comment.
| let Some(patch_name) = &patch.metadata.name else { | |
| let Some(object_name) = &override.metadata.name else { |
|
|
||
| let deserialized_patch = | ||
| patch | ||
| .clone() |
There was a problem hiding this comment.
note: If we need to clone every single override anyway, we should instead take an owned value (in both functions). Taking a reference is signaling that no owned data is needed, which is not true. We should make this clear when these functions are called.
|
|
||
| /// Using [`serde_yaml`] to generate the test data | ||
| fn generate_service_account() -> ServiceAccount { | ||
| serde_yaml::from_str( |
There was a problem hiding this comment.
note: Again, I would like to see this (and all other inline YAML strings) getting moved into actual YAML files.
| /// Generate the test data programmatically (as operators would normally do) | ||
| fn generate_stateful_set() -> StatefulSet { |
There was a problem hiding this comment.
question: But, do we need to? I feel like this can also be a static YAML file (to actually only test the merging/overrides).
Techassi
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
Description
Part of stackabletech/issues#712
Decision: https://github.com/stackabletech/decisions/issues/64
Needs stackabletech/documentation#807 for the documentation part
The actual code change is oversee-able, most of the added lines are tests.
Definition of Done Checklist
Author
Reviewer
Acceptance