Address PR comments

kubernetes-sigs · Jun 25, 2018 · 2716561 · 2716561
1 parent 00fb0b7
commit 2716561
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 20 deletions.
diff --git a/cmd/controller-scaffold/cmd/api.go b/cmd/controller-scaffold/cmd/api.go
@@ -18,11 +18,10 @@ import (
 	"fmt"
 	"log"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 
-	"os/exec"
-
 	"github.com/spf13/cobra"
 	flag "github.com/spf13/pflag"
 	"sigs.k8s.io/controller-tools/pkg/scaffold"

diff --git a/pkg/scaffold/controller/controllertest.go b/pkg/scaffold/controller/controllertest.go
@@ -50,26 +50,17 @@ import (
 	"testing"
 	"time"
 
-	{{ if .Resource.CreateExampleReconcileBody }}
 	"github.com/onsi/gomega"
 	"golang.org/x/net/context"
+	{{ if .Resource.CreateExampleReconcileBody -}}
 	appsv1 "k8s.io/api/apps/v1"
+	{{ end -}}
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	{{ .Resource.Group }}{{ .Resource.Version }} "{{ .Repo }}/pkg/apis/{{ .Resource.Group }}/{{ .Resource.Version }}"
-	{{ else }}
-	"github.com/onsi/gomega"
-	"golang.org/x/net/context"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/manager"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
-	{{ .Resource.Group }}{{ .Resource.Version }} "{{ .Repo }}/pkg/apis/{{ .Resource.Group }}/{{ .Resource.Version }}"
-	{{ end }}
 )
 
 var c client.Client

diff --git a/pkg/scaffold/resource/resource.go b/pkg/scaffold/resource/resource.go
@@ -51,13 +51,13 @@ type Resource struct {
 // Validate checks the Resource values to make sure they are valid.
 func (r *Resource) Validate() error {
 	if len(r.Group) == 0 {
-		return fmt.Errorf("must specify group")
+		return fmt.Errorf("group cannot be empty")
 	}
 	if len(r.Version) == 0 {
-		return fmt.Errorf("must specify version")
+		return fmt.Errorf("version cannot be empty")
 	}
 	if len(r.Kind) == 0 {
-		return fmt.Errorf("Must specify kind")
+		return fmt.Errorf("kind cannot be empty")
 	}
 
 	rs := inflect.NewDefaultRuleset()
@@ -67,17 +67,17 @@ func (r *Resource) Validate() error {
 
 	groupMatch := regexp.MustCompile("^[a-z]+$")
 	if !groupMatch.MatchString(r.Group) {
-		return fmt.Errorf("--group must match regex ^[a-z]+$ but was (%s)", r.Group)
+		return fmt.Errorf("group must match ^[a-z]+$ (was %s)", r.Group)
 	}
+
 	versionMatch := regexp.MustCompile("^v\\d+(alpha\\d+|beta\\d+)?$")
 	if !versionMatch.MatchString(r.Version) {
 		return fmt.Errorf(
-			"--version has bad format. must match ^v\\d+(alpha\\d+|beta\\d+)*$.  "+
-				"e.g. v1alpha1,v1beta1,v1 but was (%s)", r.Version)
+			"version must match ^v\\d+(alpha\\d+|beta\\d+)?$ (was %s)", r.Version)
 	}
 
 	if r.Kind != inflect.Camelize(r.Kind) {
-		return fmt.Errorf("Kind must be camelcase expected: %s was: (%s)", inflect.Camelize(r.Kind), r.Kind)
+		return fmt.Errorf("Kind must be camelcase (expected %s was %s)", inflect.Camelize(r.Kind), r.Kind)
 	}
 
 	return nil

diff --git a/pkg/scaffold/resource/resource_test.go b/pkg/scaffold/resource/resource_test.go
@@ -23,43 +23,58 @@ var _ = Describe("Resource", func() {
 		It("should fail if the Group is not specified", func() {
 			instance := &Resource{Version: "v1", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring("group cannot be empty"))
 		})
 
 		It("should fail if the Group is not all lowercase", func() {
 			instance := &Resource{Group: "Crew", Version: "v1", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring("group must match ^[a-z]+$ (was Crew)"))
 		})
 
 		It("should fail if the Group contains non-alpha characters", func() {
 			instance := &Resource{Group: "crew1", Version: "v1", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring("group must match ^[a-z]+$ (was crew1)"))
 		})
 
 		It("should fail if the Version is not specified", func() {
 			instance := &Resource{Group: "crew", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring("version cannot be empty"))
 		})
 
 		It("should fail if the Version does not match the version format", func() {
 			instance := &Resource{Group: "crew", Version: "1", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring(
+				`version must match ^v\d+(alpha\d+|beta\d+)?$ (was 1)`))
 
 			instance = &Resource{Group: "crew", Version: "1beta1", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring(
+				`version must match ^v\d+(alpha\d+|beta\d+)?$ (was 1beta1)`))
 
 			instance = &Resource{Group: "crew", Version: "a1beta1", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring(
+				`version must match ^v\d+(alpha\d+|beta\d+)?$ (was a1beta1)`))
 
 			instance = &Resource{Group: "crew", Version: "v1beta", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring(
+				`version must match ^v\d+(alpha\d+|beta\d+)?$ (was v1beta)`))
 
 			instance = &Resource{Group: "crew", Version: "v1beta1alpha1", Kind: "FirstMate"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring(
+				`version must match ^v\d+(alpha\d+|beta\d+)?$ (was v1beta1alpha1)`))
 		})
 
 		It("should fail if the Kind is not specified", func() {
 			instance := &Resource{Group: "crew", Version: "v1"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring("kind cannot be empty"))
 		})
 
 		It("should fail if the Kind is not camel cased", func() {
@@ -73,9 +88,13 @@ var _ = Describe("Resource", func() {
 
 			instance = &Resource{Group: "crew", Kind: "firstMate", Version: "v1"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring(
+				`Kind must be camelcase (expected FirstMate was firstMate)`))
 
 			instance = &Resource{Group: "crew", Kind: "firstmate", Version: "v1"}
 			Expect(instance.Validate()).NotTo(Succeed())
+			Expect(instance.Validate().Error()).To(ContainSubstring(
+				`Kind must be camelcase (expected Firstmate was firstmate)`))
 		})
 
 		It("should default the Resource by pluralizing the Kind", func() {