feat: Adds a warning when a defined object override was not applied - #1264
feat: Adds a warning when a defined object override was not applied#1264sweb wants to merge 2 commits into
Conversation
6bf58f7 to
2f10879
Compare
sweb
changed the title
2f10879 to
2349216
Compare
d9924ba to
5e5d619
Compare
5e5d619 to
d073166
Compare
Maleware
moved this from Development: Waiting for Review
to Refinement: In Progress
in Stackable Engineering
Maleware
moved this from Refinement: In Progress
to Development: In Review
in Stackable Engineering
Maleware
left a comment
Maleware
left a comment
There was a problem hiding this comment.
Up to the comment, LGTM thanks!
| /// | ||
| /// * `client` - The client which is used to access Kubernetes | ||
| pub async fn delete_orphaned_resources(self, client: &Client) -> Result<()> { | ||
| self.warn_about_unmatched_object_overrides(); |
There was a problem hiding this comment.
Thanks for the call. I understand it better now. I'd like to have a comment here, why the decision was to put in delete_orphaned_resources something along the lines:
Did it here because last step as it's only about resources which didn't had a match. Did so to not clutter Snafu warnings directly related to malformed CRDs.
There was a problem hiding this comment.
addressed in 9a2adee
@Maleware I thought a bit more about this - I think you are right that we should warn earlier. My initial thinking was that an object override would not make an apply step fail but that is wrong. When I tried to add it earlier I ran into the issue that you actually only get to match against all possible overrides, once all applies have been done. There are some checks we could do before, but not all of them.
To keep this small, I'd leave it at this, but I think this deserves further improvements - but from what I looked at, that would be more invasive than this change.
| warn!( | ||
| "The objectOverride at index {index} (apiVersion: {api_version:?}, kind: \ | ||
| {kind:?}, metadata.name: {name:?}, metadata.namespace: {namespace:?}) did not \ | ||
| match any object created for this cluster and therefore had no effect. Please \ | ||
| check that apiVersion, kind and metadata.name are correct and that \ | ||
| metadata.namespace is set to {cluster_namespace:?}.", | ||
| cluster_namespace = self.namespace, | ||
| ); |
There was a problem hiding this comment.
We generally prefer providing dynamic values as fields/attributes (by mentioning them before the message text in the warn! macro). This allows consumers (like OpenTelemetry collectors) to access those fields as structured data instead of only having access to a message string.
|
|
||
| - `ClusterResources` now warns about `objectOverrides` entries that did not match any of the objects it created ([#1264]). | ||
| - BREAKING: To enable this, `apply_deep_merge` now returns whether the merge matched the base object and `ObjectOverrides::apply_to` | ||
| returns the indices of the entries that matched. |
There was a problem hiding this comment.
We usually add changes as a single entry (or split place multiple entries into multiple sections).
So I would recommend combining this entry into a single one or split it up into "Added" and "Changed" section.
3 participants
Description
Adds a warning when a defined object override was not applied.
Optional: Adds the base namespace as default when the namespace of the override is not providedI removed this as it seems too much of a change.Definition of Done Checklist
Author
Reviewer
Acceptance