From 02bb4f9ffd9c72d9bf3b77058a385a83ea932b41 Mon Sep 17 00:00:00 2001 From: "Shanshan.Ying" Date: Mon, 5 Jun 2023 19:08:12 +0800 Subject: [PATCH 1/2] fix: hasLabels and matchingLabels step on each other --- pkg/client/options.go | 20 ++++++++++---- pkg/client/options_test.go | 55 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/pkg/client/options.go b/pkg/client/options.go index 50a461f1cc..d81bf25de9 100644 --- a/pkg/client/options.go +++ b/pkg/client/options.go @@ -513,8 +513,15 @@ type MatchingLabels map[string]string // ApplyToList applies this configuration to the given list options. func (m MatchingLabels) ApplyToList(opts *ListOptions) { // TODO(directxman12): can we avoid reserializing this over and over? - sel := labels.SelectorFromValidatedSet(map[string]string(m)) - opts.LabelSelector = sel + if opts.LabelSelector == nil { + opts.LabelSelector = labels.NewSelector() + } + // If there's already a selector, we need to AND the two together. + noValidSel := labels.SelectorFromValidatedSet(map[string]string(m)) + reqs, _ := noValidSel.Requirements() + for _, req := range reqs { + opts.LabelSelector = opts.LabelSelector.Add(req) + } } // ApplyToDeleteAllOf applies this configuration to the given an List options. @@ -528,14 +535,17 @@ type HasLabels []string // ApplyToList applies this configuration to the given list options. func (m HasLabels) ApplyToList(opts *ListOptions) { - sel := labels.NewSelector() + if opts.LabelSelector == nil { + opts.LabelSelector = labels.NewSelector() + } + // TODO: ignore invalid labels will result in an empty selector. + // This is inconsistent to the that of MatchingLabels. for _, label := range m { r, err := labels.NewRequirement(label, selection.Exists, nil) if err == nil { - sel = sel.Add(*r) + opts.LabelSelector = opts.LabelSelector.Add(*r) } } - opts.LabelSelector = sel } // ApplyToDeleteAllOf applies this configuration to the given an List options. diff --git a/pkg/client/options_test.go b/pkg/client/options_test.go index efba976c4f..4850b9121f 100644 --- a/pkg/client/options_test.go +++ b/pkg/client/options_test.go @@ -237,6 +237,19 @@ var _ = Describe("MatchingLabels", func() { expectedErrMsg := `values[0][k]: Invalid value: "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui": must be no more than 63 characters` Expect(err.Error()).To(Equal(expectedErrMsg)) }) + + It("Should add matchingLabels to existing selector", func() { + listOpts := &client.ListOptions{} + + matchingLabels := client.MatchingLabels(map[string]string{"k": "v"}) + matchingLabels2 := client.MatchingLabels(map[string]string{"k2": "v2"}) + + matchingLabels.ApplyToList(listOpts) + Expect(listOpts.LabelSelector.String()).To(Equal("k=v")) + + matchingLabels2.ApplyToList(listOpts) + Expect(listOpts.LabelSelector.String()).To(Equal("k=v,k2=v2")) + }) }) var _ = Describe("FieldOwner", func() { @@ -292,3 +305,45 @@ var _ = Describe("ForceOwnership", func() { Expect(*o.Force).To(Equal(true)) }) }) + +var _ = Describe("HasLabels", func() { + const invalidLongKey = "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui" + + It("Should produce hasLabels in given order", func() { + listOpts := &client.ListOptions{} + + hasLabels := client.HasLabels([]string{"labelApe", "labelFox"}) + hasLabels.ApplyToList(listOpts) + Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox")) + }) + + It("Should produce an empty selector when given invalid input", func() { + listOpts := &client.ListOptions{} + // construct a valid hasLabels selector + hasLabel := client.HasLabels([]string{invalidLongKey}) + hasLabel.ApplyToList(listOpts) + Expect(listOpts.LabelSelector.Empty()).To(BeTrue()) + }) + + It("Should add hasLabels to existing hasLabels selector", func() { + listOpts := &client.ListOptions{} + + hasLabel := client.HasLabels([]string{"labelApe"}) + hasLabel.ApplyToList(listOpts) + + hasOtherLabel := client.HasLabels([]string{"labelFox"}) + hasOtherLabel.ApplyToList(listOpts) + Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox")) + }) + + It("Should add hasLabels to existing matchingLabels", func() { + listOpts := &client.ListOptions{} + + matchingLabels := client.MatchingLabels(map[string]string{"k": "v"}) + matchingLabels.ApplyToList(listOpts) + + hasLabel := client.HasLabels([]string{"labelApe"}) + hasLabel.ApplyToList(listOpts) + Expect(listOpts.LabelSelector.String()).To(Equal("k=v,labelApe")) + }) +}) From cf20e7c28e5da18dcd74aacd2ba83860aa2d22db Mon Sep 17 00:00:00 2001 From: "Shanshan.Ying" Date: Mon, 12 Jun 2023 10:34:01 +0800 Subject: [PATCH 2/2] remove testcase with invalid input --- pkg/client/options_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/client/options_test.go b/pkg/client/options_test.go index 4850b9121f..a1208f1bfd 100644 --- a/pkg/client/options_test.go +++ b/pkg/client/options_test.go @@ -307,8 +307,6 @@ var _ = Describe("ForceOwnership", func() { }) var _ = Describe("HasLabels", func() { - const invalidLongKey = "axahm2EJ8Phiephe2eixohbee9eGeiyees1thuozi1xoh0GiuH3diewi8iem7Nui" - It("Should produce hasLabels in given order", func() { listOpts := &client.ListOptions{} @@ -317,14 +315,6 @@ var _ = Describe("HasLabels", func() { Expect(listOpts.LabelSelector.String()).To(Equal("labelApe,labelFox")) }) - It("Should produce an empty selector when given invalid input", func() { - listOpts := &client.ListOptions{} - // construct a valid hasLabels selector - hasLabel := client.HasLabels([]string{invalidLongKey}) - hasLabel.ApplyToList(listOpts) - Expect(listOpts.LabelSelector.Empty()).To(BeTrue()) - }) - It("Should add hasLabels to existing hasLabels selector", func() { listOpts := &client.ListOptions{}