feat: SSA based dependent resource matching and create/update #1928
Conversation
|
@metacosm will introduce a separate feature flag for matching, and have that set for legacy while have set the update/create to SSA. I think that is a good compromise. |
Added the additional feature flag, but defaulted it also to true. Thus by default SSA based matcher will be turned on. We can always just tell the users to turn it off with the flag. |
There was a problem hiding this comment.
LGTM as far as I can tell without diving into the managed fields structure. This is why it's important to document some of the steps so that future maintainers will be able to understand some of the things the current implementation does, in particular with respect to the more exotic fields.
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static void keepOnlyManagedFields(Map<String, Object> result, |
There was a problem hiding this comment.
Is there no way to return the pruned map instead of passing it as a parameter?
There was a problem hiding this comment.
probably is, just would need a refactor. If you insist can do that. A separate PR?
|
|
||
| // list entries referenced by key, or when "k:" prefix is used | ||
| @SuppressWarnings("unchecked") | ||
| private static void handleListKeyEntrySet(Map<String, Object> result, |
There was a problem hiding this comment.
Please add documentation as to what this method is doing and why.
|
|
||
| // set values, the "v:" prefix | ||
| @SuppressWarnings("rawtypes") | ||
| private static void handleSetValues(Map<String, Object> result, Map<String, Object> actualMap, |
There was a problem hiding this comment.
Please add documentation about what this method is doing and why.
| return isKeyPrefixedSkippingDotKey(managedEntrySet, K_PREFIX); | ||
| } | ||
|
|
||
| private static boolean isKeyPrefixedSkippingDotKey(Set<Map.Entry<String, Object>> managedEntrySet, |
There was a problem hiding this comment.
This method is weird. Why do we only look at the first element of the set? Why do we only skip the dot key if it's the first element? Please add documentation as to what this method does and why.
| } | ||
| // this should not happen in theory | ||
| if (targetManagedFields.size() > 1) { | ||
| log.debug("More than one field manager exists with name: {} in resource: {} with name: {} ", |
There was a problem hiding this comment.
Maybe this should throw an exception if this isn't supposed to happen, instead of simply logging as something is probably very wrong in this case.
yep, tbh there are some corner cases that are probably not covered, but should be very rare, I expect that we will need to do some forward fixes. But will document the the current one more in depth according to your comments. |
|
Kudos, SonarCloud Quality Gate passed! |
The PR has two main parts:
changed the default way how the dependent resources create/update the target resource. From now on it will use by default server side apply.
It provides a new default matcher the
SSABasedGenericKubernetesResourceMatcher. The intention with this new matcher is to provide a better default. This won't be perfect, thus for sure there are corner cases that are not covered. Either we will patch it or just advise to implement a specific matcher for the users use-case.