There are multiple aspects of this:

1. Practical: the change from this PR would simply not work in this scenario: the config map. (as in the test) is created prior the dependent resource is reconciled. From the PR the logic would recognize that the config map is there so won't create / update it. Now we delete the primary resource, since we did not create/update the resource the owner reference it not added, so the config map is not deleted. This is inconsistent with the current and desireg behavior, so simply wrong.

The scenario as tested by the test is wrong, though. The ConfigMap exists so it shouldn't be created, that's understood. However, the ConfigMapDependentResource defines a desired state which doesn't match the existing CM. So, according to the proper behavior, the CM should be updated. With the current implementation, it is not and that is a bug. The test is wrong and actually shows that the current implementation is not doing what's expected in this instance.

2. The semantical aspect:  we added createifNotExists flag, especially to cover this situation, that we don't override the resource based on desired if we did not create it. Again the devil is in details, we did not create it since there is not owner reference (in other words the resource is not related to primary), so based on this flag we don't want to override it. This is what the failing test is testing, and it is correct.

I don't agree with this. The desired method should prevail in all instances since it encapsulates what the operator thinks the proper state is. If an existing state is to be preserved, then the desired method should take this into account, not a flag that breaks the rest of the behavior. Or this resource shouldn't be a dependent resource if a desired state cannot appropriately be computed from the primary resource. I guess I should have paid more attention when that flag was added because I think it is wrong and goes against the rest of the design, especially with it being true by default. Not only that but the name of the constant and the flag do not appropriately convey what will happen.

3. performance:
   For exmaple on failing test this aspect does not give any performance gain, we receiving the target resource id, from the primaryToSecondary index in constant time, and getting it from cache directly, this is about the same performance. Note that we usually getting a constant number of resources, looping through is also constant, so from performance perspective of a single dependent is negligible. If you compare the execution time with http call there will be a factor of million.

Performance matter everywhere. Indeed, most of our processing will be faster than a network call but is that a valid excuse to have inefficient code? Also, all those little things add up. In any case, the core of the issue is not so much performance than the facts that:

1. users should be able to override this public method without breaking things as was previously the case
2. the flag that supposedly deals with SSA creation of resources really doesn't and is wrong with the rest of the framework design. Either it needs to be removed, set to false by default and/or properly justified with a valid use case. Again, if the desired state cannot be computed from a primary resource then the associated resource should NOT be a dependent resource, that's the base of the dependent resource feature.

So in general, I think this PR should not be merged, since simply breaks the logic point (1.). Would require to change the actual logic for (2.). We can further discuss performance gains further in case we solve (1,2), but those are really quastinable also in practice / real world examples.

I argue that the logic exhibited in the breaking test is wrong and should be reverted and the associated flag removed as such use cases should not be modeled as dependent resources. Adding that flag was a mistake we need to fix.