Support for the other label selector fields about cluster-api-addon-provider-helm HOT 7 CLOSED

/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

from cluster-api-addon-provider-helm.

/assign

from cluster-api-addon-provider-helm.

@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.

from cluster-api-addon-provider-helm.

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.

from cluster-api-addon-provider-helm.

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!

from cluster-api-addon-provider-helm.

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.

from cluster-api-addon-provider-helm.

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!

from cluster-api-addon-provider-helm.