-
-
Notifications
You must be signed in to change notification settings - Fork 16
feat: Support objectOverrides
#1118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 11 commits
00fd1b7
38a0996
d87791f
aaf74c2
22b1732
ec5b882
a46cada
058a828
58ab5ca
45a2ec5
7cb1b89
bf1d753
cb0de44
4873ac2
5071be8
713ea10
9f5019b
e28d64e
0d6c134
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,21 @@ All notable changes to this project will be documented in this file. | |
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
|
|
||
| - Support `objectOverrides` ([#1118]). | ||
|
|
||
| ### Changed | ||
|
|
||
| - BREAKING: `ClusterResources` now requires the objects added to implement `DeepMerge`. | ||
| This is very likely a stackable-operator internal change, but technically breaking ([#1118]). | ||
|
|
||
| ### Removed | ||
|
|
||
| - BREAKING: `ClusterResources` no longer derives `Eq` and `PartialEq` ([#1118]). | ||
|
||
|
|
||
| [#1118]: https://github.com/stackabletech/operator-rs/pull/1118 | ||
|
|
||
| ## [0.100.3] - 2025-10-31 | ||
|
|
||
| ### Changed | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,148 @@ | ||
| use crate::crd::listener::listeners::v1alpha1::ListenerSpec; | ||
| use k8s_openapi::{DeepMerge, merge_strategies}; | ||
|
|
||
| use crate::crd::listener::listeners::v1alpha1::{ | ||
| Listener, ListenerIngress, ListenerPort, ListenerSpec, ListenerStatus, | ||
| }; | ||
|
|
||
| impl ListenerSpec { | ||
| pub(super) const fn default_publish_not_ready_addresses() -> Option<bool> { | ||
| Some(true) | ||
| } | ||
| } | ||
|
|
||
| impl DeepMerge for Listener { | ||
| fn merge_from(&mut self, other: Self) { | ||
| DeepMerge::merge_from(&mut self.metadata, other.metadata); | ||
| DeepMerge::merge_from(&mut self.spec, other.spec); | ||
| DeepMerge::merge_from(&mut self.status, other.status); | ||
| } | ||
| } | ||
|
|
||
| impl DeepMerge for ListenerSpec { | ||
| fn merge_from(&mut self, other: Self) { | ||
| DeepMerge::merge_from(&mut self.class_name, other.class_name); | ||
| 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); | ||
| }, | ||
| ); | ||
|
Comment on lines
24
to
39
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: Please add two dev comments to quickly explain what this code does and why we need this specialized merge.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I struggle to find anything that is not already on the trait docs in https://docs.rs/k8s-openapi/latest/k8s_openapi/trait.DeepMerge.html
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was able to add two comments in e28d64e |
||
| DeepMerge::merge_from( | ||
| &mut self.publish_not_ready_addresses, | ||
| other.publish_not_ready_addresses, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| impl DeepMerge for ListenerStatus { | ||
| fn merge_from(&mut self, other: Self) { | ||
| DeepMerge::merge_from(&mut self.service_name, other.service_name); | ||
| 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); | ||
| }, | ||
| ); | ||
|
Comment on lines
50
to
65
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: Same as above. |
||
| } | ||
| } | ||
|
|
||
| impl DeepMerge for ListenerIngress { | ||
| fn merge_from(&mut self, other: Self) { | ||
| DeepMerge::merge_from(&mut self.address, other.address); | ||
| self.address_type = other.address_type; | ||
| merge_strategies::map::granular( | ||
| &mut self.ports, | ||
| other.ports, | ||
| |current_item, other_item| { | ||
| DeepMerge::merge_from(current_item, other_item); | ||
| }, | ||
| ); | ||
|
Comment on lines
+73
to
+79
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: Again, same as above. |
||
| } | ||
| } | ||
|
|
||
| impl DeepMerge for ListenerPort { | ||
| fn merge_from(&mut self, other: Self) { | ||
| DeepMerge::merge_from(&mut self.name, other.name); | ||
| DeepMerge::merge_from(&mut self.port, other.port); | ||
| DeepMerge::merge_from(&mut self.protocol, other.protocol); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn deep_merge_listener() { | ||
| let mut base: ListenerSpec = serde_yaml::from_str( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: Can we please move those YAML contents into actual files (for example located at |
||
| " | ||
| className: my-listener-class | ||
| extraPodSelectorLabels: | ||
| foo: bar | ||
| ports: | ||
| - name: http | ||
| port: 8080 | ||
| protocol: http | ||
| - name: https | ||
| port: 8080 | ||
| protocol: https | ||
| # publishNotReadyAddresses defaults to true | ||
| ", | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let patch: ListenerSpec = serde_yaml::from_str( | ||
| " | ||
| className: custom-listener-class | ||
| extraPodSelectorLabels: | ||
| foo: overridden | ||
| extra: label | ||
| ports: | ||
| - name: https | ||
| port: 8443 | ||
| publishNotReadyAddresses: false | ||
| ", | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| base.merge_from(patch); | ||
|
|
||
| let expected: ListenerSpec = serde_yaml::from_str( | ||
| " | ||
| className: custom-listener-class | ||
| extraPodSelectorLabels: | ||
| foo: overridden | ||
| extra: label | ||
| ports: | ||
| - name: http | ||
| port: 8080 | ||
| protocol: http | ||
| - name: https | ||
| port: 8443 # overridden | ||
| protocol: https | ||
| publishNotReadyAddresses: false | ||
| ", | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(base, expected); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ pub mod kvp; | |
| pub mod logging; | ||
| pub mod memory; | ||
| pub mod namespace; | ||
| pub mod patchinator; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Maybe
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like the "cleverness" of the code to apply arbitrary patches to any k8s resource. And yeah, I also like the name ^^ |
||
| pub mod pod_utils; | ||
| pub mod product_config_utils; | ||
| pub mod product_logging; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||
| use kube::api::DynamicObject; | ||||||||||||||
| use schemars::JsonSchema; | ||||||||||||||
| use serde::{Deserialize, Serialize}; | ||||||||||||||
|
|
||||||||||||||
| use crate::utils::crds::raw_object_list_schema; | ||||||||||||||
|
|
||||||||||||||
| #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] | ||||||||||||||
| #[serde(rename_all = "camelCase")] | ||||||||||||||
| pub struct ObjectOverrides { | ||||||||||||||
| /// A list of generic Kubernetes objects, which are merged onto the objects that the operator | ||||||||||||||
| /// creates. | ||||||||||||||
|
Comment on lines
+10
to
+11
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about this?
Suggested change
It makes it clear what is merged onto what |
||||||||||||||
| /// | ||||||||||||||
| /// List entries are arbitrary YAML objects, which need to be valid Kubernetes objects. | ||||||||||||||
| /// | ||||||||||||||
| /// Read the [Object overrides documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/overrides#object-overrides) | ||||||||||||||
| /// for more information. | ||||||||||||||
| #[serde(default)] | ||||||||||||||
| #[schemars(schema_with = "raw_object_list_schema")] | ||||||||||||||
| pub object_overrides: Vec<DynamicObject>, | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Seeing that this struct only contains a single field, this is make sense to turn this into a tuple struct instead? This could also enable us to implement
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we could have #[derive(Clone, Debug, Deserialize, Default, JsonSchema, Serialize, PartialEq)]
pub struct ObjectOverrides2(
/// A list of generic Kubernetes objects, which are merged onto the objects that the operator
/// creates.
///
/// List entries are arbitrary YAML objects, which need to be valid Kubernetes objects.
///
/// Read the [Object overrides documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/overrides#object-overrides)
/// for more information.
#[schemars(schema_with = "raw_object_list_schema")]
Vec<DynamicObject>,
);and than call them as #[serde(flatten)]
object_overrides: ObjectOverrides,
#[serde(default)]
object_overrides_2: ObjectOverrides2,I think we should optimize for ease of use, not sure which one that is. |
||||||||||||||
| } | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I think we should add a small explanation how this works/what this does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a bit of content in 9f5019b