-
Notifications
You must be signed in to change notification settings - Fork 217
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
✨ feat: Accessing old object on watchEvent
#517
base: main
Are you sure you want to change the base?
✨ feat: Accessing old object on watchEvent
#517
Conversation
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
I don't know if I have updated docs correctly, Also in Delete event we can fill OldObject instead of Object, Since we can Called removed object 'old' |
@nabokihms Can you review this? |
@mhkarimi1383 yes, sure. I've just put it in the queue. |
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.
I left a couple of suggestions. One more thing is that the unit test is also required for the feature.
Co-authored-by: Maksim Nabokikh <max.nabokih@gmail.com>
I will add unit test for that later on, I have not worked with Golang unit tests alot :) Can you give me a context of how should I test this thing? I mean testing the input and output is good enough or I have to do a more advaced test |
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@nabokihms Hi, I have fixed linter and unit test error.! The only left thing is the feature unit tests :) |
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Any updates on this? My project development blocked 😕 |
@nabokihms
|
"oldObject": { | ||
"apiVersion": "apps/v1", | ||
"kind": "DaemonSet", | ||
"metadata": { | ||
"name": "flannel", | ||
"namespace": "d8-flannel" | ||
}, | ||
"spec": { | ||
"selector": { | ||
"matchLabels": { | ||
"app": "flannel" | ||
} | ||
}, | ||
"template": { | ||
"metadata": { | ||
"labels": { | ||
"app": "flannel", | ||
"tier": "old-node" | ||
} | ||
}, | ||
"spec": { | ||
"containers": [ | ||
{ | ||
"args": [ | ||
"--ip-masq", | ||
"--kube-subnet-mgr" | ||
], | ||
"image": "flannel:v0.11", | ||
"name": "kube-flannel", | ||
"securityContext": { | ||
"privileged": true | ||
} | ||
} | ||
], | ||
"hostNetwork": true, | ||
"imagePullSecrets": [ | ||
{ | ||
"name": "registry" | ||
} | ||
], | ||
"terminationGracePeriodSeconds": 5 | ||
} | ||
}, | ||
"updateStrategy": { | ||
"type": "RollingUpdate" | ||
} | ||
} | ||
}, |
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.
"oldObject": { | |
"apiVersion": "apps/v1", | |
"kind": "DaemonSet", | |
"metadata": { | |
"name": "flannel", | |
"namespace": "d8-flannel" | |
}, | |
"spec": { | |
"selector": { | |
"matchLabels": { | |
"app": "flannel" | |
} | |
}, | |
"template": { | |
"metadata": { | |
"labels": { | |
"app": "flannel", | |
"tier": "old-node" | |
} | |
}, | |
"spec": { | |
"containers": [ | |
{ | |
"args": [ | |
"--ip-masq", | |
"--kube-subnet-mgr" | |
], | |
"image": "flannel:v0.11", | |
"name": "kube-flannel", | |
"securityContext": { | |
"privileged": true | |
} | |
} | |
], | |
"hostNetwork": true, | |
"imagePullSecrets": [ | |
{ | |
"name": "registry" | |
} | |
], | |
"terminationGracePeriodSeconds": 5 | |
} | |
}, | |
"updateStrategy": { | |
"type": "RollingUpdate" | |
} | |
} | |
}, |
This is not the right doc. The doc is about Kubernetes object modification, not about binding contexts.
The right doc is here https://github.com/flant/shell-operator/blob/main/docs/src/HOOKS.md#kubernetes-binding-context-example
I fixed the linter, but PR doesn't work because there are other parts missing. For example, to finally see an old object in a hook, this file requires changes as well And probably this one shell-operator/pkg/hook/controller/kubernetes_bindings_controller.go Lines 294 to 329 in 1d4c4d9
One more thing, without adding tests, we will not be able to merge the PR. |
Overview
By using OldObject input from K8s we will have access to oldObject when modified
What this PR does / why we need it
Closes #515
I need this here: mhkarimi1383/reflector#3
Sometimes we need to have access to older object specially if we are managing our own CRD
Special notes for your reviewer