fix: correct parameters order in GenericKubernetesResourceMatcher call to JsonDiff.asJson #3009
Conversation
…l to JsonDiff.asJson
|
The tests fail, so this seems to break the current behavior. |
|
yes, see the discussion around that: |
|
@xstefank, is there anything I can do about it? How can I run/check those tests? |
| if (!ignoreList.isEmpty()) { | ||
| return nodeIsChildOf(diff, ignoreList); | ||
| } | ||
| return ADD.equals(diff.get(OP).asText()); |
There was a problem hiding this comment.
If you swap the parameter order, I'm pretty sure you have to change the operation here. The issue is that there can be remove or replace, if I recall correctly. So that's maybe why the current parameter order was chosen.
We have used a copy of this code in our own operator to identify changes that need a pod restart. And we never swapped the parameter order due to the (single) add vs. (multiple) remove / replace operations in the diff.
Also, be aware that default values added by K8s will show up here. This is a known limitation of the generic matcher (and is fixed in the SSA based matcher, by filtering out all fields that are not managed by the operator). So you have to anticipate changes in here, that are not just triggered by changes in your desired state, but also from the actual side.
There was a problem hiding this comment.
@Donnerbart I must admit I have no experience with the JOSDK source code, but AFAIK, desired represents the to-be state and actual the as-is state in Kubernetes.
That being said, you might be right, but I don’t think the parameter order of JsonDiff.asJson() depends on the caller’s needs. As I understand it, the outcome of JsonDiff.asJson()—once applied with JsonPatch.apply() to the source—should result in the target parameter.
In JOSDK terms, the patch calculated with JsonDiff.asJson(desiredNode, actualNode)—once applied to actualNode—should result in desiredNode.
As far as I’ve tested, it doesn’t behave that way with the current JsonDiff.asJson() call.
But I may have understood it wrong, and if so, I apologize for wasting everybody’s time with this.
4 participants
#2997