Merge pull request kubernetes#2753 from spiffxp/make-test-fixes

Get `make test` working again

Author: k8s-ci-robot

.gitignore

@@ -24,3 +24,6 @@ Session.vim
 
 # Files generated by JetBrains IDEs, e.g. IntelliJ IDEA
 .idea/
+
+# hack/test-go.sh writes files here if ARTIFACTS is unset
+tmp/

Makefile

@@ -86,10 +86,10 @@ test-go-integration: ## Runs Golang integration tests
 
 .PHONY: tools
 
-KEP_TOOLS ?=
+WHAT ?= kepctl kepify
 
-tools: ## Compiles a set of KEP tools, specified by $KEP_TOOLS
-	./compile-tools $(KEP_TOOLS)
+tools: ## Installs all KEP tools, can select via e.g. WHAT=kepctl
+	./compile-tools $(WHAT)
 
 ##@ Dependencies
 

api/approval.go

@@ -94,19 +94,6 @@ type PRRMilestone struct {
 
 type PRRHandler Parser
 
-func NewPRRHandler() (*PRRHandler, error) {
-	handler := &PRRHandler{}
-
-	approvers, err := FetchPRRApprovers()
-	if err != nil {
-		return nil, errors.Wrap(err, "fetching PRR approvers")
-	}
-
-	handler.PRRApprovers = approvers
-
-	return handler, nil
-}
-
 // TODO(api): Make this a generic parser for all `Document` types
 func (p *PRRHandler) Parse(in io.Reader) (*PRRApproval, error) {
 	scanner := bufio.NewScanner(in)

api/groups.go

@@ -29,19 +29,65 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
-const (
-	// URLs
-	sigListURL     = "https://raw.githubusercontent.com/kubernetes/community/master/sigs.yaml"
-	ownersAliasURL = "https://raw.githubusercontent.com/kubernetes/enhancements/master/OWNERS_ALIASES"
+// GroupFetcher is responsible for getting informationg about groups in the
+// Kubernetes Community
+type GroupFetcher interface {
+	// FetchGroups returns the list of valid Kubernetes Community groups
+	// e.g. (SIGs, WGs, UGs, Committees)
+	FetchGroups() ([]string, error)
+	// FetchPRRApprovers returns the list of valid PRR Approvers
+	FetchPRRApprovers() ([]string, error)
+}
 
-	// Aliases
-	prrApproversAlias = "prod-readiness-approvers"
-)
+// DefaultGroupFetcher returns the default GroupFetcher, which depends on GitHub
+func DefaultGroupFetcher() GroupFetcher {
+	return &RemoteGroupFetcher{
+		GroupsListURL:     "https://raw.githubusercontent.com/kubernetes/community/master/sigs.yaml",
+		OwnersAliasesURL:  "https://raw.githubusercontent.com/kubernetes/enhancements/master/OWNERS_ALIASES",
+		PRRApproversAlias: "prod-readiness-approvers",
+	}
+}
+
+// MockGroupFetcher returns the given Groups and PRR Approvers
+type MockGroupFetcher struct {
+	Groups       []string
+	PRRApprovers []string
+}
+
+var _ GroupFetcher = &MockGroupFetcher{}
+
+func NewMockGroupFetcher(groups, prrApprovers []string) GroupFetcher {
+	return &MockGroupFetcher{Groups: groups, PRRApprovers: prrApprovers}
+}
+
+func (f *MockGroupFetcher) FetchGroups() ([]string, error) {
+	result := make([]string, len(f.Groups))
+	copy(result, f.Groups)
+	return result, nil
+}
+
+func (f *MockGroupFetcher) FetchPRRApprovers() ([]string, error) {
+	result := make([]string, len(f.PRRApprovers))
+	copy(result, f.PRRApprovers)
+	return result, nil
+}
+
+// RemoteGroupFetcher returns Groups and PRR Approvers from remote sources
+type RemoteGroupFetcher struct {
+	// Basically kubernetes/community/sigs.yaml
+	GroupsListURL string
+	// Basically kubernetes/enhancements/OWNERS_ALIASES
+	OwnersAliasesURL string
+	// The alias name to look for in OWNERS_ALIASES
+	PRRApproversAlias string
+}
+
+var _ GroupFetcher = &RemoteGroupFetcher{}
 
-// FetchGroups returns a list of Kubernetes governance groups
-// (SIGs, Working Groups, User Groups).
-func FetchGroups() ([]string, error) {
-	resp, err := http.Get(sigListURL)
+// FetchGroups returns the list of valid Kubernetes Community groups as
+// fetched from a remote source
+func (f *RemoteGroupFetcher) FetchGroups() ([]string, error) {
+	resp, err := http.Get(f.GroupsListURL)
 	if err != nil {
 		return nil, errors.Wrap(err, "fetching SIG list")
 	}
@@ -78,8 +124,8 @@ func FetchGroups() ([]string, error) {
 }
 
 // FetchPRRApprovers returns a list of PRR approvers.
-func FetchPRRApprovers() ([]string, error) {
-	resp, err := http.Get(ownersAliasURL)
+func (f *RemoteGroupFetcher) FetchPRRApprovers() ([]string, error) {
+	resp, err := http.Get(f.OwnersAliasesURL)
 	if err != nil {
 		return nil, errors.Wrap(err, "fetching owners aliases")
 	}
@@ -108,7 +154,7 @@ func FetchPRRApprovers() ([]string, error) {
 	}
 
 	var result []string
-	result = append(result, config.Data[prrApproversAlias]...)
+	result = append(result, config.Data[f.PRRApproversAlias]...)
 
 	if len(result) == 0 {
 		return nil, errors.New("retrieved zero PRR approvers, which is unexpected")

api/proposal.go

@@ -76,15 +76,6 @@ type Proposal struct {
 	Contents string `json:"markdown" yaml:"-"`
 }
 
-func (p *Proposal) Validate() error {
-	v := validator.New()
-	if err := v.Struct(p); err != nil {
-		return errors.Wrap(err, "running validation")
-	}
-
-	return nil
-}
-
 func (p *Proposal) IsMissingMilestone() bool {
 	return p.LatestMilestone == ""
 }
@@ -95,26 +86,6 @@ func (p *Proposal) IsMissingStage() bool {
 
 type KEPHandler Parser
 
-func NewKEPHandler() (*KEPHandler, error) {
-	handler := &KEPHandler{}
-
-	groups, err := FetchGroups()
-	if err != nil {
-		return nil, errors.Wrap(err, "fetching groups")
-	}
-
-	handler.Groups = groups
-
-	approvers, err := FetchPRRApprovers()
-	if err != nil {
-		return nil, errors.Wrap(err, "fetching PRR approvers")
-	}
-
-	handler.PRRApprovers = approvers
-
-	return handler, nil
-}
-
 // TODO(api): Make this a generic parser for all `Document` types
 func (k *KEPHandler) Parse(in io.Reader) (*Proposal, error) {
 	scanner := bufio.NewScanner(in)
@@ -153,16 +124,75 @@ func (k *KEPHandler) Parse(in io.Reader) (*Proposal, error) {
 		return kep, errors.Wrap(err, "unmarshalling YAML")
 	}
 
-	if valErr := kep.Validate(); valErr != nil {
-		k.Errors = append(k.Errors, errors.Wrap(valErr, "validating KEP"))
-		return kep, errors.Wrap(valErr, "validating KEP")
+	if err := k.validateStruct(kep); err != nil {
+		k.Errors = append(k.Errors, err)
+		return kep, fmt.Errorf("validating KEP: %w", err)
 	}
 
 	kep.ID = hash(kep.OwningSIG + ":" + kep.Title)
 
 	return kep, nil
 }
 
+// validateStruct returns an error if the given Proposal has invalid fields
+// as defined by struct tags, or nil if there are no invalid fields
+func (k *KEPHandler) validateStruct(p *Proposal) error {
+	v := validator.New()
+	return v.Struct(p)
+}
+
+// validateGroups returns errors for each invalid group (e.g. SIG) in the given
+// Proposal, or nil if there are no invalid groups
+func (k *KEPHandler) validateGroups(p *Proposal) []error {
+	var errs []error
+	validGroups := make(map[string]bool)
+	for _, g := range k.Groups {
+		validGroups[g] = true
+	}
+	for _, g := range p.ParticipatingSIGs {
+		if _, ok := validGroups[g]; !ok {
+			errs = append(errs, fmt.Errorf("invalid participating-sig: %s", g))
+		}
+	}
+	if _, ok := validGroups[p.OwningSIG]; !ok {
+		errs = append(errs, fmt.Errorf("invalid owning-sig: %s", p.OwningSIG))
+	}
+	return errs
+}
+
+// validatePRRApprovers returns errors for each invalid PRR Approver in the
+// given Proposal, or nil if there are no invalid PRR Approvers
+func (k *KEPHandler) validatePRRApprovers(p *Proposal) []error {
+	var errs []error
+	validPRRApprovers := make(map[string]bool)
+	for _, a := range k.PRRApprovers {
+		validPRRApprovers[a] = true
+	}
+	for _, a := range p.PRRApprovers {
+		if _, ok := validPRRApprovers[a]; !ok {
+			errs = append(errs, fmt.Errorf("invalid prr-approver: %s", a))
+		}
+	}
+	return errs
+}
+
+// Validate returns errors for each reason the given proposal is invalid,
+// or nil if it is valid
+func (k *KEPHandler) Validate(p *Proposal) []error {
+	var allErrs []error
+
+	if err := k.validateStruct(p); err != nil {
+		allErrs = append(allErrs, fmt.Errorf("struct-based validation: %w", err))
+	}
+	if errs := k.validateGroups(p); errs != nil {
+		allErrs = append(allErrs, errs...)
+	}
+	if errs := k.validatePRRApprovers(p); errs != nil {
+		allErrs = append(allErrs, errs...)
+	}
+	return allErrs
+}
+
 type Milestone struct {
 	Alpha  string `json:"alpha" yaml:"alpha"`
 	Beta   string `json:"beta" yaml:"beta"`

cmd/kepify/main.go

@@ -25,8 +25,6 @@ import (
 	"path/filepath"
 	"strings"
 
-	"github.com/pkg/errors"
-
 	"k8s.io/enhancements/api"
 )
 
@@ -72,8 +70,24 @@ func main() {
 		os.Exit(1)
 	}
 
+	fetcher := api.DefaultGroupFetcher()
+
+	groups, err := fetcher.FetchGroups()
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "unable to fetch groups: %v", err)
+		os.Exit(1)
+	}
+
+	prrApprovers, err := fetcher.FetchPRRApprovers()
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "unable to fetch PRR approvers: %v", err)
+		os.Exit(1)
+	}
+
+	kepHandler := &api.KEPHandler{Groups: groups, PRRApprovers: prrApprovers}
+
 	// Parse the files
-	proposals, err := parseFiles(files)
+	proposals, err := parseFiles(kepHandler, files)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "error parsing files: %q\n", err)
 		os.Exit(1)
@@ -112,14 +126,9 @@ func findMarkdownFiles(dirPath *string) ([]string, error) {
 	return files, err
 }
 
-func parseFiles(files []string) (api.Proposals, error) {
+func parseFiles(parser *api.KEPHandler, files []string) (api.Proposals, error) {
 	var proposals api.Proposals
 	for _, filename := range files {
-		parser, err := api.NewKEPHandler()
-		if err != nil {
-			return nil, errors.Wrap(err, "creating new KEP handler")
-		}
-
 		file, err := os.Open(filename)
 		if err != nil {
 			return nil, fmt.Errorf("could not open file: %v", err)

compile-tools

@@ -29,10 +29,11 @@ setup_env() {
 
   if [[ -z "${TOOL_BIN:-}" ]]; then
     if [ -n "${GOBIN:-}" ]; then
-      export TOOL_BIN="${GOBIN}"
+      TOOL_BIN="${GOBIN}"
     else
-      export TOOL_BIN="${GOPATH}/bin"
+      TOOL_BIN="$(go env GOPATH)/bin"
     fi
+    export TOOL_BIN
   fi
 
   export PATH="${PATH}:${TOOL_BIN}"
@@ -89,20 +90,22 @@ main() {
   setup_env
   check_deps
 
-  if [ $# -gt 0 ]; then
-    for tool in "$@"; do
-      if [[ ${tool} =~ github.com\/ ]]; then
-        compile "${tool}"
-      else
-        compile_with_flags "${tool}"
-      fi
-    done
-  else
-    for tool in "${TOOLS[@]}"; do
-      echo "Compiling default tools..."
-      compile_with_flags "${tool}"
-    done
+  if [ $# = 0 ]; then
+    # default to all tools
+    set -- "${TOOLS[@]}"
   fi
+
+  for tool; do
+    if ! (printf '%s\n' "${TOOLS[@]}" | grep -q "^${tool}$"); then
+      echo "Skipping unrecognized tool name: ${tool}" >&2
+      continue
+    fi
+    if [[ ${tool} =~ github.com\/ ]]; then
+      compile "${tool}"
+    else
+      compile_with_flags "${tool}"
+    fi
+  done
 }
 
 main "$@"