make test: fix TestProposalValidate invalid sig

Move validation logic from Proposal to KEPHandler, since Proposals don't
know which Groups and PRRApprovers are valid

Author: spiffxp

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 == ""
 }
@@ -153,16 +144,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"`

pkg/proposal/create.go

@@ -80,12 +80,12 @@ func Create(opts *CreateOpts) error {
 
 	populateProposal(kep, opts)
 
-	err := kep.Validate()
-	if err != nil {
-		return err
+	errs := r.KEPHandler.Validate(kep)
+	if errs != nil {
+		return fmt.Errorf("invalid kep: %v", errs)
 	}
 
-	err = createKEP(kep, opts)
+	err := createKEP(kep, opts)
 	if err != nil {
 		return err
 	}

pkg/repo/repo_test.go

@@ -29,22 +29,25 @@ import (
 
 func TestProposalValidate(t *testing.T) {
 	testcases := []struct {
-		name        string
-		file        string
-		expectError bool
+		name         string
+		file         string
+		expectErrors bool
 	}{
 		{
-			name:        "valid KEP passes validate",
-			file:        "testdata/valid-kep.yaml",
-			expectError: false,
+			name:         "valid KEP passes validate",
+			file:         "testdata/valid-kep.yaml",
+			expectErrors: false,
 		},
 		{
-			name:        "invalid KEP fails validate for owning-sig",
-			file:        "testdata/invalid-kep.yaml",
-			expectError: true,
+			name:         "invalid KEP fails validate for owning-sig",
+			file:         "testdata/invalid-kep.yaml",
+			expectErrors: true,
 		},
 	}
 
+	parser := api.KEPHandler{}
+	parser.Groups = []string{"sig-api-machinery"}
+
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
 			b, err := ioutil.ReadFile(tc.file)
@@ -54,9 +57,9 @@ func TestProposalValidate(t *testing.T) {
 			err = yaml.Unmarshal(b, &p)
 			require.NoError(t, err)
 
-			err = p.Validate()
-			if tc.expectError {
-				require.Error(t, err)
+			errs := parser.Validate(&p)
+			if tc.expectErrors {
+				require.NotEmpty(t, errs)
 			}
 
 			require.NoError(t, err)