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

Support for the other label selector fields #15

Closed
hama-25179 opened this issue Jan 26, 2023 · 7 comments · Fixed by #60
Closed

Support for the other label selector fields #15

hama-25179 opened this issue Jan 26, 2023 · 7 comments · Fixed by #60
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@hama-25179
Copy link
Contributor

User Story

In creating a manifest for HelmChartProxy, I would like to add support for other fields of the labelselector, such as matchExpressions.

Anything else you would like to add:

This issue is related to PR #14.

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 26, 2023
@Jont828
Copy link
Contributor

Jont828 commented Feb 2, 2023

/triage accepted

Seems reasonable to me. The only consideration is that we'd need to make sure the other fields can be parsed into the list options like in this line

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Feb 2, 2023
@Jont828
Copy link
Contributor

Jont828 commented Feb 27, 2023

/assign

@Jont828
Copy link
Contributor

Jont828 commented Mar 10, 2023

@hama-25179 I have my hands full at the moment with trying to set up the staging repo and image publishing for this project so I'm not able to work on this issue yet. I do have some ideas on how I would go about implementing it. Would you be interested in picking up this issue? If so, I'm happy to share my thoughts on how to get started.

@hama-25179
Copy link
Contributor Author

I'd like to work on this issue, but I don't have much time available for that work right now. I'd like to hear your ideas, as I may have more time in the next month or so.

@Jont828
Copy link
Contributor

Jont828 commented Mar 30, 2023

Sorry for the delay, but here's the line where we do the label selection. The main part is client.MatchingLabels. In the source code for client.MatchingLabels, there's also a MatchingFieldSelector option we can use. That selector leads to additional interfaces where I think we can try to implement the extra logic.

Those are my initial thoughts -- it does strike me as odd that it doesn't handle this out of the box. This solution definitely seems more involved so maybe I missed something that would simplify it. But let me know what you think!

@hama-25179
Copy link
Contributor Author

hama-25179 commented Apr 10, 2023

I thought that to use matchExpressions of the Label selector, I could pass the entire labelselector (client.MatchingLabelsSelector) to the Client.List() instead of client.MatchingLabels.
I would change this line as follows.

labelselector, err := metav1.LabelSelectorAsSelector(&selector)
if err != nil {
	return nil, err
}
if err := r.Client.List(ctx, clusterList, client.InNamespace(namespace), client.MatchingLabelsSelector{Selector: labelselector}); err != nil {
 ...

I'd like to confirm this comment.
If Label selector isn't defined, the behavior is to match all. (The same applies to matchLabels and matchiExpressions)
If this behavior is OK, I don't think it is necessary to validate key and value. let me know what you think.

@Jont828
Copy link
Contributor

Jont828 commented Apr 20, 2023

Sounds good, I think that makes sense to me. And yes, it looks like if you don't define any labels to match it should match everything. I'll take a look at your PR and hopefully we can get it merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
3 participants