-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix(merge): ensure to have same data type while performing 3-way merge #154
base: master
Are you sure you want to change the base?
Conversation
This PR adds the check to perform 3-way merge only if datatype matches to observed state data types else error will be returned. **Bug Description**: merge func silently allows to merge when different data types are provided for 3-way merge and this leads merge data will not match to expected data. Following is an example: ```go observed := map[string]interface{}{ "key1": "value1", "key2": "value2", } desired := map[string]string{ "key1": "value1", "key2": "value2", } got := map[string]interface{}{ "key1": "value1", "key2": "value2", } ``` Signed-off-by: mittachaitu <[email protected]>
hi @mittachaitu , do you have maybe any example when it causes an issue ? It would be good to know some example. I will try to add the same to https://github.com/metacontroller/metacontroller |
Signed-off-by: mittachaitu <[email protected]>
Hi @grzesuav, I faced an issue when trying to merge K8s configmap. Following is the code snippet to reproduce the issue observed := map[string]interface{}{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": map[string]interface{}{
"name": "node-cm1",
"namespace": "metac",
},
"data": map[string]interface{}{
"node.properties": "data1",
},
}
desired := map[string]interface{}{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": map[string]interface{}{
"name": "node-cm1",
"namespace": "metac",
},
"data": map[string]string{
"node.properties": "data2",
},
}
// Calling Merge will return observed state instead of observed
got, err := Merge(observed, nil, desired)
if err != nil {
// return error
}
// Output will be the same as an observed state which is not expected
got := map[string]interface{}{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": map[string]interface{}{
"name": "node-cm1",
"namespace": "metac",
},
"data": map[string]string{
"node.properties": "data1",
},
} UnitTest case also available to test the same changes |
@mittachaitu thanks :) will try to test it onmetacontroller :) |
@mittachaitu may I ask where the issue arises ? As I understand it is because json data is differently serialized in golang ? Do you have maybe json example of kubernetes ConfigMap ? |
@grzesuav Basically we are using metac as k8s library code for d-operators. How we reached here: Note: When we call Sample JSON example: apiVersion: v1
data:
node.properties: |
key1: value1
kind: ConfigMap
metadata:
creationTimestamp: "2021-03-17T07:30:57Z"
name: propel-node-cm1
namespace: propel
resourceVersion: "54933120"
selfLink: /api/v1/namespaces/propel/configmaps/propel-node-cm1
uid: c7d3870f-92bd-45ae-81f1-ae680cb35025 |
This PR adds the check to perform 3-way merge only if datatype
matches to observed state data types else error will be returned.
Bug Description:
merge func silently allows to merge when different data types are
provided for 3-way merge and this leads merge data will not match to
expected data. Following is an example(not an expected behaviour):
The following error will be reported to the caller after this patch:
Can't merge desired changes: [data]: Expecting last applied as map[string]interface{}, got map[string]string
Signed-off-by: mittachaitu [email protected]