Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KeyValuePairs::extend implements a nonsense merge #887

Closed
nightkr opened this issue Oct 11, 2024 · 0 comments · Fixed by #888 · May be fixed by #889
Closed

KeyValuePairs::extend implements a nonsense merge #887

nightkr opened this issue Oct 11, 2024 · 0 comments · Fixed by #888 · May be fixed by #889

Comments

@nightkr
Copy link
Member

nightkr commented Oct 11, 2024

Affected version

sble-operator 0.76.0

Current and expected behavior

If you try to merge the following KVP sets:

a: b
b: a
---
a: a
b: b

and then build a k8s_openapi label map from it you get

a: b
b: b

Which is nonsense, I'd expect one of them to take precedence (and according to convention/ObjectMetaBuilder::with_label docs) it should be the latter ({a: a, b: b}).

Possible solution

Ultimately, this comes down to that we try to store reified KeyValuePairs in a BTreeSet. This isn't what sets exist for.

As a "quick fix" we could change the backing store for KeyValuePairs to BTreeMap<Key, V> (though its Deref<BTreeSet> impl makes that problematic too, on top of a bunch of lifetime issues). Ultimately I'd like to rip the band-aid and just kill KeyValuePair.

Additional context

No response

Environment

No response

Would you like to work on fixing this bug?

yes

@nightkr nightkr self-assigned this Oct 11, 2024
@nightkr nightkr moved this to Development: In Progress in Stackable Engineering Oct 11, 2024
@Techassi Techassi moved this from Development: In Progress to Development: In Review in Stackable Engineering Oct 16, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 17, 2024
* Replace KeyValuePairs' internal BTreeSet with BTreeMap

Fixes #887

* Rename PairAlreadyExists => KeyAlreadyExists

* Changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment