Avoid skipping a storage due to identical string representations #5934
Conversation
fmt.Sprintf("%v", thing) will use a fmt.Stringer implementation in thing, which means
that we cannot easily assume that this value is going to be unique here and can be used
to deduplicate the storages.
Instead: Use a `map[interface{}]struct{}` as we anyways have an interface, and interfaces
are perfectly fine map keys.
|
Quick playground: https://go.dev/play/p/UgWKHYRn17H |
There was a problem hiding this comment.
Thanks!
Hmm, yes, this is probably better; although, I wonder if this is too strict of a comparison. It uses the pointer to the value as the key, so if we have 2 identical storage configurations with different pointers (like they are configured in different automation policies) then it won't deduplicate properly.
Maybe that is fine. It's probably better to clean twice than to skip cleaning entirely. (Unless the storage mechanism is expensive, like a cloud DB or something.)
My original hope was that the %v would be able to capture a representation of the storage config in a general way.
Anyway, I think overall, this change is probably good.
If you think the issue I mention above would be a problem in your case, though, let me know and we can figure something out.
Yeah. Well. :) |
I think that's a good goal actually, but maybe that should then be a (optional) requirement on the storage implementation: If you implement interface I'm wondering whether it could be done backwards-compatible, but I really think the foot-shooting potential of falling back to |
Co-authored-by: Matt Holt <[email protected]>
|
As mentioned in Slack, I have a proper fix inbound, I think. Stay tuned in the next day or so for that! |
|
Gently closing this in favor of #5940 -- we can reopen this if needed, of course. Thanks for helping! 😀 |
fmt.Sprintf("%v", thing) will use a fmt.Stringer implementation in thing, which means that we cannot easily assume that this value is going to be unique here and can be used to deduplicate the storages.
Instead: Use a
map[interface{}]struct{}as we anyways have an interface, and interfaces are perfectly fine map keys.As a side-effect: Implementors of storage can now implement fmt.Stringer safely, and get the expected output here.