Delete Cluster Scoped Resources #92
Conversation
muralov
commented
Sep 13, 2023
•
muralov
commented
- Cluster role and binding were not deleted as k8s garbage collector cannot delete if namespace scoped owns cluster scoped
- Add tests
- Improve conditions
* Cluster role and binding were not deleted as k8s garbage collector cannot if namespace scoped owns cluster scoped * Add tests * Improve conditions
kyma-bot
added
the
size/L
| } | ||
| eventing.Status.SetPublisherProxyConditionToFalse( | ||
| eventingv1alpha1.ConditionReasonDeleted, | ||
| eventingv1alpha1.ConditionPublisherProxyDeletedMessage) | ||
|
|
There was a problem hiding this comment.
Should this function be covered in the unit test as well?
There was a problem hiding this comment.
do you mean the function eventing.Status.SetPublisherProxyConditionToFalse?
There was a problem hiding this comment.
I'm sorry, no, I meant handleEventingDeletion.
There was a problem hiding this comment.
It seems there is no unit test yet. Deletion is covered with Integration Tests.
It'd be too much if I implement now as there are multiple other cases out-of-scope. Can we separate it a follow-up issue to increase code coverage later? Anyway we need to improve int tests err cases. I wanted to write, then decided to do it later.
api/v1alpha1/eventing_types.go
Outdated
| @@ -65,7 +65,7 @@ const ( | |||
| ConditionReasonEventMeshSubManagerReady ConditionReason = "EventMeshSubscriptionManagerReady" | |||
| ConditionReasonEventMeshSubManagerFailed ConditionReason = "EventMeshSubscriptionManagerFailed" | |||
| ConditionReasonEventMeshSubManagerStopFailed ConditionReason = "EventMeshSubscriptionManagerStopFailed" | |||
| ConditionReasonEventMeshSubManagerStop ConditionReason = "Stopped" | |||
| ConditionReasonEventMeshSubManagerStopped ConditionReason = "Stopped" | |||
There was a problem hiding this comment.
The ConditionReason for all other cases is EventMeshSubscriptionManager<something>. For the sake of consistency, can we have EventMeshSubscriptionManagerStopped here?
Otherwise, if none of them need to be EventMesh specific, can the other reasons have that part removed, so it can be reused for NATS?
There was a problem hiding this comment.
Reason stopped is actually common for NATS and EventMesh. Thus, changed to general name.
