api: Add error handling and test for retrieving a stage's PRR approver

Signed-off-by: Stephen Augustus <[email protected]>

Author: justaugustus

api/approval.go

@@ -53,17 +53,38 @@ func (prr *PRRApproval) Validate() error {
 	return nil
 }
 
-func (prr *PRRApproval) ApproverForStage(stage string) string {
+func (prr *PRRApproval) ApproverForStage(stage string) (string, error) {
+	isValidStage := IsOneOf(stage, ValidStages)
+	if !isValidStage {
+		return "", ErrKEPStageIsInvalid(stage)
+	}
+
+	if prr.Alpha == nil && prr.Beta == nil && prr.Stable == nil {
+		return "", ErrPRRMilestonesAllEmpty
+	}
+
 	switch stage {
 	case "alpha":
-		return prr.Alpha.Approver
+		if prr.Alpha == nil {
+			return "", ErrPRRMilestoneIsNil
+		}
+
+		return prr.Alpha.Approver, nil
 	case "beta":
-		return prr.Beta.Approver
+		if prr.Beta == nil {
+			return "", ErrPRRMilestoneIsNil
+		}
+
+		return prr.Beta.Approver, nil
 	case "stable":
-		return prr.Stable.Approver
+		if prr.Stable == nil {
+			return "", ErrPRRMilestoneIsNil
+		}
+
+		return prr.Stable.Approver, nil
 	}
 
-	return ""
+	return "", ErrPRRApproverUnknown
 }
 
 // TODO(api): Can we refactor the proposal `Milestone` to retrieve this?
@@ -112,13 +133,3 @@ func (p *PRRHandler) Parse(in io.Reader) (*PRRApproval, error) {
 
 	return approval, nil
 }
-
-func IsOneOf(check string, slice []string) bool {
-	for _, v := range slice {
-		if v == check {
-			return true
-		}
-	}
-
-	return false
-}

api/approval_test.go

@@ -0,0 +1,103 @@
+/*
+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
+
+    htcp://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 api_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/enhancements/api"
+)
+
+var (
+	prrMilestoneWithApprover = &api.PRRMilestone{
+		Approver: "@wojtek-t",
+	}
+
+	validAlphaPRR = &api.PRRApproval{
+		Alpha: prrMilestoneWithApprover,
+	}
+
+	validBetaPRR = &api.PRRApproval{
+		Beta: prrMilestoneWithApprover,
+	}
+
+	validStablePRR = &api.PRRApproval{
+		Stable: prrMilestoneWithApprover,
+	}
+
+	invalidPRR = &api.PRRApproval{}
+)
+
+func TestPRRApproval_ApproverForStage(t *testing.T) {
+	testcases := []struct {
+		name         string
+		stage        string
+		prr          *api.PRRApproval
+		wantApprover string
+		wantErr      bool
+	}{
+		{
+			name:    "invalid: incorrect stage",
+			stage:   "badstage",
+			prr:     validAlphaPRR,
+			wantErr: true,
+		},
+		{
+			name:         "invalid: missing approver",
+			stage:        "alpha",
+			prr:          invalidPRR,
+			wantApprover: "",
+			wantErr:      true,
+		},
+		{
+			name:         "valid: alpha",
+			stage:        "alpha",
+			prr:          validAlphaPRR,
+			wantApprover: "@wojtek-t",
+			wantErr:      false,
+		},
+		{
+			name:         "valid: beta",
+			stage:        "beta",
+			prr:          validBetaPRR,
+			wantApprover: "@wojtek-t",
+			wantErr:      false,
+		},
+		{
+			name:         "valid: stable",
+			stage:        "stable",
+			prr:          validStablePRR,
+			wantApprover: "@wojtek-t",
+			wantErr:      false,
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			hasErr := false
+			approver, err := tc.prr.ApproverForStage(tc.stage)
+			if err != nil {
+				hasErr = true
+			}
+
+			require.Equal(t, tc.wantErr, hasErr)
+			require.Equal(t, tc.wantApprover, approver)
+		},
+		)
+	}
+}

api/document.go

@@ -46,3 +46,14 @@ type Parser struct {
 
 	Errors []error
 }
+
+// TODO: Can we come up with a better name/location for this?
+func IsOneOf(check string, slice []string) bool {
+	for _, v := range slice {
+		if v == check {
+			return true
+		}
+	}
+
+	return false
+}

api/error.go

@@ -0,0 +1,50 @@
+/*
+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 api
+
+import (
+	"fmt"
+
+	"github.com/pkg/errors"
+)
+
+var (
+	// PRR errors
+
+	ErrPRRMilestonesAllEmpty error = errors.New(
+		"none of the PRR milestones are populated",
+	)
+
+	ErrPRRMilestoneIsNil error = errors.New(
+		"the selected PRR milestone stage is not populated",
+	)
+
+	ErrPRRApproverUnknown error = errors.New(
+		"an unknown error occurred while trying to determine a PRR approver",
+	)
+)
+
+// KEP errors
+func ErrKEPStageIsInvalid(stage string) error {
+	return errors.New(
+		fmt.Sprintf(
+			"the specified stage (%s) should be one of the following: %v",
+			stage,
+			ValidStages,
+		),
+	)
+}

api/proposal.go

@@ -29,6 +29,12 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
+var ValidStages = []string{
+	"alpha",
+	"beta",
+	"stable",
+}
+
 type Proposals []*Proposal
 
 func (p *Proposals) AddProposal(proposal *Proposal) {

pkg/kepctl/kepctl.go

@@ -415,15 +415,21 @@ func (c *Client) loadKEPFromYaml(kepPath string) (*api.Proposal, error) {
 		fmt.Fprintf(c.Err, "WARNING: could not parse prod readiness request for KEP %s: %s\n", p.Number, approval.Error)
 	}
 
-	approver := approval.ApproverForStage(p.Stage)
+	approver, err := approval.ApproverForStage(p.Stage)
+	if err != nil {
+		return nil, errors.Wrapf(err, "getting PRR approver for %s stage", p.Stage)
+	}
+
 	for _, a := range p.PRRApprovers {
 		if a == approver {
 			approver = ""
 		}
 	}
+
 	if approver != "" {
 		p.PRRApprovers = append(p.PRRApprovers, approver)
 	}
+
 	return &p, nil
 }
 

pkg/kepval/approval.go

@@ -71,7 +71,14 @@ func ValidatePRR(kep *api.Proposal, h *api.PRRHandler, prrDir string) error {
 		return errors.Wrapf(prr.Error, "%v has an error", prrFilepath)
 	}
 
-	stagePRRApprover := prr.ApproverForStage(kep.Stage)
+	stagePRRApprover, err := prr.ApproverForStage(kep.Stage)
+	if err != nil {
+		return errors.Wrapf(err, "getting PRR approver for %s stage", kep.Stage)
+	}
+
+	if stagePRRApprover == "" {
+		return errors.New("PRR approver cannot be empty")
+	}
 
 	if strings.HasPrefix(stagePRRApprover, "@") {
 		stagePRRApprover = strings.TrimPrefix(stagePRRApprover, "@")