Fixing notification groups deep equal bug #43
Conversation
…p the order of creation, required different kind of comparison
…p the order of creation, required different kind of comparison
There was a problem hiding this comment.
therefore a different type of comparison between spec and status is needed.
Could you elaborate on what is this "different type of comparison"? It's a bit hard to understand just by reading the code
And nice find discovering that the API doesn't return notification groups ordered. I'm not sure if your solution works as expected though, maps are not ordered in golang, so I'm not sure if using a map is helping here
One easy way to make sure that your function is working is by adding a unit test :)
Notifications-Groups are unique by their group-by arrays. So I create map of group-by->[]Notification and compare, then, for the Notification list I create map of integration->Notification and compare. I agree about the unit-test, but since it's blocking you let's add an issue about it and merge this PR. |
Thanks for the explanation!
Well, I'm a bit concerned about a pattern that I'm noticing here 😬. Whenever we deploy the operator in an environment we need to fight against a series of bugs. Having so many bugs is what is blocking us the most, and not this specific PR. I'm completely fine with waiting longer to deploy the operator in other environments if in return we are creating a culture of not delivering stuff untested. |
NP, I'll talk with Oded about prioritize adding tests. Currently focusing on adding validations. |
It turns out that NotificationGroups and Notifications within each NotificationGroup do not maintain the creation order, therefore a different type of comparison between spec and status is needed.