Skip to content

Commit

Permalink
馃悰 hasLabels and matchingLabels step on each other (#2363)
Browse files Browse the repository at this point in the history
* fix: hasLabels and matchingLabels step on each other

* remove testcase with invalid input
  • Loading branch information
shanshanying committed Jun 12, 2023
1 parent 4419a85 commit eb78e57
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 5 deletions.
20 changes: 15 additions & 5 deletions pkg/client/options.go
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
45 changes: 45 additions & 0 deletions pkg/client/options_test.go
Expand Up @@ -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() {
Expand Down Expand Up @@ -292,3 +305,35 @@ var _ = Describe("ForceOwnership", func() {
Expect(*o.Force).To(Equal(true))
})
})

var _ = Describe("HasLabels", func() {
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 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"))
})
})

0 comments on commit eb78e57

Please sign in to comment.