Fix namespaces deletion handling #159
Conversation
Following #156 some conflicts are happening, do you think you might have some time to take a look? |
tr-aheiev
force-pushed
the
fix-namespaces-deletion-handling
branch
from
5465f70 to
dbabc96
Compare
|
Hi! I updated the code. Not sure - do I need to bump Chart version each time to pass tests? |
Yes, I cannot change the GitHub workflows, so need to bump it to let it pass :/ |
| @@ -116,143 +116,6 @@ def test_on_field_data_sync(self): | |||
| {"key": "newvalue"}, | |||
| ) | |||
|
|
|||
| def test_on_field_data_ns_deleted(self): | |||
There was a problem hiding this comment.
The test has been removed, could you clarify? If the changes fixes an issue, we should have a regression tests, otherwise if it is adding a new behaviour, we should ensure it works as expected
There was a problem hiding this comment.
Yes, you are right. Previously 'on_field_data' function was doing little more than now, so there was additional test for it and I removed it after it changes. But now when I added new functionality into 'namespace_watcher' function, seems I need to write some additional test for it. I will try to do this today/tomorrow.
There was a problem hiding this comment.
Thanks! Really appreciate!
There was a problem hiding this comment.
It took me a bit more time to fully understand the root purpose of the removed test and how the other tests work.
Initially, I assumed that the handler for namespace removal would eliminate the situation that the test was checking. However, that’s not the case - it only significantly reduces the likelihood of it occurring. In kopf, handlers for different objects (such as ClusterSecret and Namespace) don’t run sequentially; they execute in parallel. This means there’s still a very small chance that when 'on_field_data' is triggered, 'namespace_watcher' might still be in progress but not fully completed.
So, it seems that this test is not entirely redundant, even now. Of course we still need an additional test for 'namespace_watcher'.
I’ll continue working on this.
There was a problem hiding this comment.
This tests has been written a long time ago, so I cannot help much on that, thanks you for investigating.
There was a problem hiding this comment.
Hello! Sorry for the delay.
I added an additional unit test for 'namespace_watcher' to verify the correct update of ClusterSecret and its cache after a namespace deletion.
I reviewed the 'test_on_field_data_ns_deleted' test once again and have now removed it. Since our handlers use synchronous functions, they execute sequentially, ensuring that namespaces changes in status fields are processed correctly. Even if a deletion occurs during the execution of 'on_fields_avoid_or_match_namespace', I have added an extra check in 'kubernetes_utils' to handle this properly and log a debug message accordingly.
I updated the code and think that now its ready for merge.
tr-aheiev
force-pushed
the
fix-namespaces-deletion-handling
branch
from
b95536f to
9f08521
Compare
…, especially in 'data.valueFrom'
Removed unnecessary unit test
tr-aheiev
force-pushed
the
fix-namespaces-deletion-handling
branch
from
9f08521 to
014cdcf
Compare
This PR removes outdated code from 'on_field_data' and simplifying secrets synchronization for both types - with inline data fields and with external secrets (with 'valueFrom' field). But after removing that code we need somehow handle namespace deletion operations - for this purpose I refactored 'namespace_watcher' function. Test for namespace creation I fixed also, but for namespace removing need to create new - not sure that can handle it myself, but will try.
All current tests are passed.