Skip to content

Conversation

@Techassi
Copy link
Member

@Techassi Techassi commented Oct 31, 2023

Fixes #614, supersedes #615, part of stackabletech/issues#188

This PR adds mechanisms to safely create key/value pairs (labels and annotations). These enable us to attach a unified set of labels to deployed resources. The set of labels make it to possible to track resources installed by us, which in turn enables us to list, modify and delete resources with our management tools, namely stackablectl, and in the future stackable-cockpit.

## Tasks - [x] Move `stackable_operator::label_selector::convert_label_selector_to_query_string` into `kvp` module - [x] Remove all temporary `unwraps` - [x] Add doc comments to all new structs, modules, types, etc... 
@Techassi Techassi self-assigned this Oct 31, 2023
@Techassi
Copy link
Member Author

We ideally want the creation of a K/V pair to be available in const environments. With the current approach making use of the (non-constant) FromStr trait, this is not possible. We might be able to sidestep this using a non-trait solution (until const in traits are stabilized and available). This however requires a more sophisticated parsing process, which looks at individual bytes without allocating.

Having K/V pair creation available in const environments would allow us to define common labels/attributes in operator-rs (or any other operator repository) which are parsed and validated during compile time. That would be huge, but at the moment rather cumbersome to implement.

The API is very close to it's final form. One open point is the usage of the new API in other operator-rs code, like the k8s resource builders. An important design decision is to make the usage through these functions convenient to use while making sure the K/V pairs are valid.

NickLarsenNZ
NickLarsenNZ previously approved these changes Nov 22, 2023
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good

@NickLarsenNZ NickLarsenNZ self-requested a review November 22, 2023 16:18
NickLarsenNZ
NickLarsenNZ previously approved these changes Nov 22, 2023
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Techassi Techassi marked this pull request as ready for review November 23, 2023 08:17
@nightkr
Copy link
Contributor

nightkr commented Dec 5, 2023

When would the former API be used? What hole is it trying to fill?

@Techassi
Copy link
Member Author

Techassi commented Dec 5, 2023

There is currently no immediate benefit from the first option, but in the future we might stumble over that kind of input, like from a file, or command line argument.

@nightkr
Copy link
Contributor

nightkr commented Dec 5, 2023

Then we can consider the problem when it actually arises. We shouldn't preemptively make it easier to do the wrong thing just in case.

Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I like the contains and contains_key that take an impl TryInto. Makes the usage really clean.

@Techassi Techassi added this pull request to the merge queue Dec 15, 2023
Merged via the queue into main with commit 38cb237 Dec 15, 2023
@Techassi Techassi deleted the feat/kvp branch December 15, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants