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

Experimenting with adapting the linkerd labels module for general use #1482

Closed
wants to merge 3 commits into from

Conversation

clux
Copy link
Member

@clux clux commented May 1, 2024

Heavy WIP. The linkerd module is good and has nice patterns, but it is not super focused to fitting our interfaces, plus it does not have a way to serialize the query string for the API so am trying to write that plus make the builder interface on expression more ergonomic. Stashing here as WIP, will maybe get to it on the weekend.

  • Selector extend with stringifiers for ListParams
  • Extend ListParams::labels_from taking a Selector
  • Builders for Expression (done 1)
  • Labels revamp
    • Allow converting from ResourceExt::labels() to something we can compare against
    • remove Labels altogether? commented out for now (arc use seems unnecessary for us)
  • combine Operator and Values? now there's the awkward representable case of In / NotIn but empty values...
  • Feature flag in jsonschema derives? If we want to embed selectors inside ObjectMeta.
  • Alternatively, more converters for selectors that can be injected into ObjectMeta

clux added 2 commits May 1, 2024 15:15
straight copy + factor out 2 std imports

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux linked an issue May 1, 2024 that may be closed by this pull request
@paologallinaharbur
Copy link

I was about to open an issue, but then I've found this PR that I think it is implementing what I was looking for!

I was looking for something similar to this go package in order to filter on client-side a list of pods given a label_selector.

If you need review/inputs let me know 😄

@Danil-Grigorev
Copy link
Member

Danil-Grigorev commented Jul 16, 2024

Maybe on the side note… I ended up implementing another version similar to linkerd in crust-gather (without knowing): - https://github.com/crust-gather/crust-gather/blob/main/src/gather/selector.rs but skipping having a LabelSelector.

So this version also lacks conversion back to original LabelSelector and the use-case it was meant for is just an ability to deserialize the definition from a string, not a tuple of matchExpressions and matchLabels keys with values… I feel like the ideal state would be to leave on user to preserve the original LabelSelector, add a trait capable of matching stuff over it, and a default implementation like the Selector presented in this PR + the ability to convert from LabelSelector to this custom Selector (like Into/From). WDYT?

PS: opened a separate issue describing in more details the use case I’m trying to cover.

@clux
Copy link
Member Author

clux commented Jul 16, 2024

So this version also lacks conversion back to original LabelSelector and the use-case it was meant for is just an ability to deserialize the definition from a string, not a tuple of matchExpressions and matchLabels keys with values… I feel like the ideal state would be to leave on user to preserve the original LabelSelector, add a trait capable of matching stuff over it, and a default implementation like the Selector presented in this PR + the ability to convert from LabelSelector to this custom Selector (like Into/From). WDYT?

Yeah, I also wanted a way to go from some arbitrary labels / ObjectMeta / K: Resource. Maybe some kind of SelectorExt trait would make sense here, as we could impl SelectorExt for ObjectMeta and hook in getters/setters that way. At any rate, do feel free to take this over. I have clearly not made a lot of progress on this issue in the last month.

@Danil-Grigorev Danil-Grigorev mentioned this pull request Jul 17, 2024
2 tasks
@clux
Copy link
Member Author

clux commented Jul 19, 2024

#1539 is a successor PR. closing this.

@clux clux closed this Jul 19, 2024
@clux clux deleted the labels-import branch July 19, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More ergonomic ListParams::labels
3 participants