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

⚠️ Decouple plugin requirements from Config interface #2047

Merged
merged 1 commit into from
Feb 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions pkg/config/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,10 @@ type Config interface {

// HasGroup checks if the provided group is the same as any of the tracked resources
HasGroup(group string) bool
// IsCRDVersionCompatible returns true if crdVersion can be added to the existing set of CRD versions.
IsCRDVersionCompatible(crdVersion string) bool
// IsWebhookVersionCompatible returns true if webhookVersion can be added to the existing set of Webhook versions.
IsWebhookVersionCompatible(webhookVersion string) bool
// ListCRDVersions returns a list of the CRD versions in use by the tracked resources
ListCRDVersions() []string
// ListWebhookVersions returns a list of the webhook versions in use by the tracked resources
ListWebhookVersions() []string

/* Plugins */

Expand Down
12 changes: 6 additions & 6 deletions pkg/config/v2/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,14 +222,14 @@ func (c cfg) HasGroup(group string) bool {
return false
}

// IsCRDVersionCompatible implements config.Config
func (c cfg) IsCRDVersionCompatible(crdVersion string) bool {
return crdVersion == apiVersion
// ListCRDVersions implements config.Config
func (c cfg) ListCRDVersions() []string {
return make([]string, 0)
}

// IsWebhookVersionCompatible implements config.Config
func (c cfg) IsWebhookVersionCompatible(webhookVersion string) bool {
return webhookVersion == apiVersion
// ListWebhookVersions implements config.Config
func (c cfg) ListWebhookVersions() []string {
return make([]string, 0)
}

// DecodePluginConfig implements config.Config
Expand Down
18 changes: 4 additions & 14 deletions pkg/config/v2/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,22 +225,12 @@ var _ = Describe("cfg", func() {
Expect(c.HasGroup("other-group")).To(BeFalse())
})

It("IsCRDVersionCompatible should return true for `v1beta1`", func() {
Expect(c.IsCRDVersionCompatible("v1beta1")).To(BeTrue())
It("ListCRDVersions should return an empty list", func() {
Expect(c.ListCRDVersions()).To(BeEmpty())
})

It("IsCRDVersionCompatible should return false for any other than `v1beta1`", func() {
Expect(c.IsCRDVersionCompatible("v1")).To(BeFalse())
Expect(c.IsCRDVersionCompatible("v2")).To(BeFalse())
})

It("IsWebhookVersionCompatible should return true for `v1beta1`", func() {
Expect(c.IsWebhookVersionCompatible("v1beta1")).To(BeTrue())
})

It("IsWebhookVersionCompatible should return false for any other than `v1beta1`", func() {
Expect(c.IsWebhookVersionCompatible("v1")).To(BeFalse())
Expect(c.IsWebhookVersionCompatible("v2")).To(BeFalse())
It("ListWebhookVersions should return an empty list", func() {
Expect(c.ListWebhookVersions()).To(BeEmpty())
})
})

Expand Down
51 changes: 28 additions & 23 deletions pkg/config/v3/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,35 +249,40 @@ func (c cfg) HasGroup(group string) bool {
return false
}

// IsCRDVersionCompatible implements config.Config
func (c cfg) IsCRDVersionCompatible(crdVersion string) bool {
return c.resourceAPIVersionCompatible("crd", crdVersion)
}
// ListCRDVersions implements config.Config
func (c cfg) ListCRDVersions() []string {
// Make a map to remove duplicates
versionSet := make(map[string]struct{})
for _, r := range c.Resources {
if r.API != nil && r.API.CRDVersion != "" {
versionSet[r.API.CRDVersion] = struct{}{}
}
}

// IsWebhookVersionCompatible implements config.Config
func (c cfg) IsWebhookVersionCompatible(webhookVersion string) bool {
return c.resourceAPIVersionCompatible("webhook", webhookVersion)
// Convert the map into a slice
versions := make([]string, 0, len(versionSet))
for version := range versionSet {
versions = append(versions, version)
}
return versions
}

func (c cfg) resourceAPIVersionCompatible(verType, version string) bool {
for _, res := range c.Resources {
var currVersion string
switch verType {
case "crd":
if res.API != nil {
currVersion = res.API.CRDVersion
}
case "webhook":
if res.Webhooks != nil {
currVersion = res.Webhooks.WebhookVersion
}
}
if currVersion != "" && version != currVersion {
return false
// ListWebhookVersions implements config.Config
func (c cfg) ListWebhookVersions() []string {
// Make a map to remove duplicates
versionSet := make(map[string]struct{})
for _, r := range c.Resources {
if r.Webhooks != nil && r.Webhooks.WebhookVersion != "" {
versionSet[r.Webhooks.WebhookVersion] = struct{}{}
}
}

return true
// Convert the map into a slice
versions := make([]string, 0, len(versionSet))
for version := range versionSet {
versions = append(versions, version)
}
return versions
}

// DecodePluginConfig implements config.Config
Expand Down
75 changes: 47 additions & 28 deletions pkg/config/v3/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package v3

import (
"errors"
"sort"
"testing"

. "github.com/onsi/ginkgo"
Expand Down Expand Up @@ -302,42 +303,60 @@ var _ = Describe("cfg", func() {
Expect(c.HasGroup("other-group")).To(BeFalse())
})

It("IsCRDVersionCompatible should return true with no tracked resources", func() {
Expect(c.IsCRDVersionCompatible("v1beta1")).To(BeTrue())
Expect(c.IsCRDVersionCompatible("v1")).To(BeTrue())
It("ListCRDVersions should return an empty list with no tracked resources", func() {
Expect(c.ListCRDVersions()).To(BeEmpty())
})

It("IsCRDVersionCompatible should return true only for matching CRD versions of tracked resources", func() {
c.Resources = append(c.Resources, resource.Resource{
GVK: resource.GVK{
Group: res.Group,
Version: res.Version,
Kind: res.Kind,
It("ListCRDVersions should return a list of tracked resources CRD versions", func() {
c.Resources = append(c.Resources,
resource.Resource{
GVK: resource.GVK{
Group: res.Group,
Version: res.Version,
Kind: res.Kind,
},
API: &resource.API{CRDVersion: "v1beta1"},
},
resource.Resource{
GVK: resource.GVK{
Group: res.Group,
Version: res.Version,
Kind: "OtherKind",
},
API: &resource.API{CRDVersion: "v1"},
},
API: &resource.API{CRDVersion: "v1beta1"},
})
Expect(c.IsCRDVersionCompatible("v1beta1")).To(BeTrue())
Expect(c.IsCRDVersionCompatible("v1")).To(BeFalse())
Expect(c.IsCRDVersionCompatible("v2")).To(BeFalse())
)
versions := c.ListCRDVersions()
sort.Strings(versions) // ListCRDVersions has no order guarantee so sorting for reproducibility
Expect(versions).To(Equal([]string{"v1", "v1beta1"}))
})

It("IsWebhookVersionCompatible should return true with no tracked resources", func() {
Expect(c.IsWebhookVersionCompatible("v1beta1")).To(BeTrue())
Expect(c.IsWebhookVersionCompatible("v1")).To(BeTrue())
It("ListWebhookVersions should return an empty list with no tracked resources", func() {
Expect(c.ListWebhookVersions()).To(BeEmpty())
})

It("IsWebhookVersionCompatible should return true only for matching webhook versions of tracked resources", func() {
c.Resources = append(c.Resources, resource.Resource{
GVK: resource.GVK{
Group: res.Group,
Version: res.Version,
Kind: res.Kind,
It("ListWebhookVersions should return a list of tracked resources webhook versions", func() {
c.Resources = append(c.Resources,
resource.Resource{
GVK: resource.GVK{
Group: res.Group,
Version: res.Version,
Kind: res.Kind,
},
Webhooks: &resource.Webhooks{WebhookVersion: "v1beta1"},
},
resource.Resource{
GVK: resource.GVK{
Group: res.Group,
Version: res.Version,
Kind: "OtherKind",
},
Webhooks: &resource.Webhooks{WebhookVersion: "v1"},
},
Webhooks: &resource.Webhooks{WebhookVersion: "v1beta1"},
})
Expect(c.IsWebhookVersionCompatible("v1beta1")).To(BeTrue())
Expect(c.IsWebhookVersionCompatible("v1")).To(BeFalse())
Expect(c.IsWebhookVersionCompatible("v2")).To(BeFalse())
)
versions := c.ListWebhookVersions()
sort.Strings(versions) // ListWebhookVersions has no order guarantee so sorting for reproducibility
Expect(versions).To(Equal([]string{"v1", "v1beta1"}))
})
})

Expand Down
35 changes: 35 additions & 0 deletions pkg/plugin/util/helpers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"sigs.k8s.io/kubebuilder/v3/pkg/config"
)

// HasDifferentCRDVersion returns true if any other CRD version is tracked in the project configuration.
func HasDifferentCRDVersion(config config.Config, crdVersion string) bool {
return hasDifferentAPIVersion(config.ListCRDVersions(), crdVersion)
}

// HasDifferentWebhookVersion returns true if any other webhook version is tracked in the project configuration.
func HasDifferentWebhookVersion(config config.Config, webhookVersion string) bool {
return hasDifferentAPIVersion(config.ListWebhookVersions(), webhookVersion)
}

func hasDifferentAPIVersion(versions []string, version string) bool {
return !(len(versions) == 0 || (len(versions) == 1 && versions[0] == version))
}
Adirio marked this conversation as resolved.
Show resolved Hide resolved
45 changes: 45 additions & 0 deletions pkg/plugin/util/helpers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"testing"

. "github.com/onsi/ginkgo"
. "github.com/onsi/ginkgo/extensions/table"
. "github.com/onsi/gomega"
)

func TestPlugin(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Plugin Util Suite")
}

var _ = Describe("hasDifferentAPIVersion", func() {
DescribeTable("should return false",
func(versions []string) { Expect(hasDifferentAPIVersion(versions, "v1")).To(BeFalse()) },
Entry("for an empty list of versions", []string{}),
Entry("for a list of only that version", []string{"v1"}),
)

DescribeTable("should return true",
func(versions []string) { Expect(hasDifferentAPIVersion(versions, "v1")).To(BeTrue()) },
Entry("for a list of only a different version", []string{"v2"}),
Entry("for a list of several different versions", []string{"v2", "v3"}),
Entry("for a list of several versions containing that version", []string{"v1", "v2"}),
)
})
3 changes: 2 additions & 1 deletion pkg/plugins/golang/v3/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"sigs.k8s.io/kubebuilder/v3/pkg/model"
"sigs.k8s.io/kubebuilder/v3/pkg/model/resource"
"sigs.k8s.io/kubebuilder/v3/pkg/plugin"
pluginutil "sigs.k8s.io/kubebuilder/v3/pkg/plugin/util"
goPlugin "sigs.k8s.io/kubebuilder/v3/pkg/plugins/golang"
"sigs.k8s.io/kubebuilder/v3/pkg/plugins/golang/v3/scaffolds"
"sigs.k8s.io/kubebuilder/v3/pkg/plugins/internal/cmdutil"
Expand Down Expand Up @@ -189,7 +190,7 @@ func (p *createAPISubcommand) Validate() error {
}

// Check CRDVersion against all other CRDVersions in p.config for compatibility.
if !p.config.IsCRDVersionCompatible(p.resource.API.CRDVersion) {
if pluginutil.HasDifferentCRDVersion(p.config, p.resource.API.CRDVersion) {
return fmt.Errorf("only one CRD version can be used for all resources, cannot add %q",
p.resource.API.CRDVersion)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/plugins/golang/v3/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"sigs.k8s.io/kubebuilder/v3/pkg/config"
"sigs.k8s.io/kubebuilder/v3/pkg/model/resource"
"sigs.k8s.io/kubebuilder/v3/pkg/plugin"
pluginutil "sigs.k8s.io/kubebuilder/v3/pkg/plugin/util"
goPlugin "sigs.k8s.io/kubebuilder/v3/pkg/plugins/golang"
"sigs.k8s.io/kubebuilder/v3/pkg/plugins/golang/v3/scaffolds"
"sigs.k8s.io/kubebuilder/v3/pkg/plugins/internal/cmdutil"
Expand Down Expand Up @@ -121,7 +122,7 @@ func (p *createWebhookSubcommand) Validate() error {
return fmt.Errorf("webhook resource already exists")
}

if !p.config.IsWebhookVersionCompatible(p.resource.Webhooks.WebhookVersion) {
if pluginutil.HasDifferentWebhookVersion(p.config, p.resource.Webhooks.WebhookVersion) {
return fmt.Errorf("only one webhook version can be used for all resources, cannot add %q",
p.resource.Webhooks.WebhookVersion)
}
Expand Down