Skip to content

Commit 0e37217

Browse files
[release-0.15] 🐛 hasLabels and matchingLabels step on each other (#2373)
* fix: hasLabels and matchingLabels step on each other * remove testcase with invalid input --------- Co-authored-by: Shanshan.Ying <shanshan.ying@apecloud.com>
1 parent 111c938 commit 0e37217

File tree

2 files changed

+60
-5
lines changed

2 files changed

+60
-5
lines changed

pkg/client/options.go

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -513,8 +513,15 @@ type MatchingLabels map[string]string
513513
// ApplyToList applies this configuration to the given list options.
514514
func (m MatchingLabels) ApplyToList(opts *ListOptions) {
515515
// TODO(directxman12): can we avoid reserializing this over and over?
516-
sel := labels.SelectorFromValidatedSet(map[string]string(m))
517-
opts.LabelSelector = sel
516+
if opts.LabelSelector == nil {
517+
opts.LabelSelector = labels.NewSelector()
518+
}
519+
// If there's already a selector, we need to AND the two together.
520+
noValidSel := labels.SelectorFromValidatedSet(map[string]string(m))
521+
reqs, _ := noValidSel.Requirements()
522+
for _, req := range reqs {
523+
opts.LabelSelector = opts.LabelSelector.Add(req)
524+
}
518525
}
519526

520527
// ApplyToDeleteAllOf applies this configuration to the given an List options.
@@ -528,14 +535,17 @@ type HasLabels []string
528535

529536
// ApplyToList applies this configuration to the given list options.
530537
func (m HasLabels) ApplyToList(opts *ListOptions) {
531-
sel := labels.NewSelector()
538+
if opts.LabelSelector == nil {
539+
opts.LabelSelector = labels.NewSelector()
540+
}
541+
// TODO: ignore invalid labels will result in an empty selector.
542+
// This is inconsistent to the that of MatchingLabels.
532543
for _, label := range m {
533544
r, err := labels.NewRequirement(label, selection.Exists, nil)
534545
if err == nil {
535-
sel = sel.Add(*r)
546+
opts.LabelSelector = opts.LabelSelector.Add(*r)
536547
}
537548
}
538-
opts.LabelSelector = sel
539549
}
540550

541551
// ApplyToDeleteAllOf applies this configuration to the given an List options.

pkg/client/options_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,19 @@ var _ = Describe("MatchingLabels", func() {
237237
expectedErrMsg := `values[0][k]: Invalid value: "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui": must be no more than 63 characters`
238238
Expect(err.Error()).To(Equal(expectedErrMsg))
239239
})
240+
241+
It("Should add matchingLabels to existing selector", func() {
242+
listOpts := &client.ListOptions{}
243+
244+
matchingLabels := client.MatchingLabels(map[string]string{"k": "v"})
245+
matchingLabels2 := client.MatchingLabels(map[string]string{"k2": "v2"})
246+
247+
matchingLabels.ApplyToList(listOpts)
248+
Expect(listOpts.LabelSelector.String()).To(Equal("k=v"))
249+
250+
matchingLabels2.ApplyToList(listOpts)
251+
Expect(listOpts.LabelSelector.String()).To(Equal("k=v,k2=v2"))
252+
})
240253
})
241254

242255
var _ = Describe("FieldOwner", func() {
@@ -292,3 +305,35 @@ var _ = Describe("ForceOwnership", func() {
292305
Expect(*o.Force).To(Equal(true))
293306
})
294307
})
308+
309+
var _ = Describe("HasLabels", func() {
310+
It("Should produce hasLabels in given order", func() {
311+
listOpts := &client.ListOptions{}
312+
313+
hasLabels := client.HasLabels([]string{"labelApe", "labelFox"})
314+
hasLabels.ApplyToList(listOpts)
315+
Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox"))
316+
})
317+
318+
It("Should add hasLabels to existing hasLabels selector", func() {
319+
listOpts := &client.ListOptions{}
320+
321+
hasLabel := client.HasLabels([]string{"labelApe"})
322+
hasLabel.ApplyToList(listOpts)
323+
324+
hasOtherLabel := client.HasLabels([]string{"labelFox"})
325+
hasOtherLabel.ApplyToList(listOpts)
326+
Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox"))
327+
})
328+
329+
It("Should add hasLabels to existing matchingLabels", func() {
330+
listOpts := &client.ListOptions{}
331+
332+
matchingLabels := client.MatchingLabels(map[string]string{"k": "v"})
333+
matchingLabels.ApplyToList(listOpts)
334+
335+
hasLabel := client.HasLabels([]string{"labelApe"})
336+
hasLabel.ApplyToList(listOpts)
337+
Expect(listOpts.LabelSelector.String()).To(Equal("k=v,labelApe"))
338+
})
339+
})

0 commit comments

Comments
 (0)