[bug-350]: Clarify role update error messages (#195)

* add failure tests and update err message of role update * change description of update command * remove print * remove test * fix grammar * update description of role commands
dell · Nov 8, 2022 · 4f4a715 · 4f4a715
1 parent 6106b3c
commit 4f4a715
Show file tree

Hide file tree

Showing 9 changed files with 43 additions and 15 deletions.
diff --git a/cmd/karavictl/cmd/role_create.go b/cmd/karavictl/cmd/role_create.go
@@ -35,8 +35,8 @@ const roleFlagSize = 5
 func NewRoleCreateCmd() *cobra.Command {
 	roleCreateCmd := &cobra.Command{
 		Use:   "create",
-		Short: "Create one or more Karavi roles",
-		Long:  `Creates one or more Karavi roles`,
+		Short: "Create one or more CSM roles",
+		Long:  `Creates one or more CSM roles`,
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()

diff --git a/cmd/karavictl/cmd/role_delete.go b/cmd/karavictl/cmd/role_delete.go
@@ -30,8 +30,8 @@ import (
 func NewRoleDeleteCmd() *cobra.Command {
 	roleDeleteCmd := &cobra.Command{
 		Use:   "delete",
-		Short: "Delete role",
-		Long:  `Delete role`,
+		Short: "Delete one or more CSM roles",
+		Long:  `Delete one or mroe CSM roles`,
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()

diff --git a/cmd/karavictl/cmd/role_get.go b/cmd/karavictl/cmd/role_get.go
@@ -30,8 +30,8 @@ import (
 func NewRoleGetCmd() *cobra.Command {
 	roleGetCmd := &cobra.Command{
 		Use:   "get",
-		Short: "Get role",
-		Long:  `Get role`,
+		Short: "Get CSM role",
+		Long:  `Get CSM role`,
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()

diff --git a/cmd/karavictl/cmd/role_list.go b/cmd/karavictl/cmd/role_list.go
@@ -27,8 +27,8 @@ import (
 func NewRoleListCmd() *cobra.Command {
 	roleListCmd := &cobra.Command{
 		Use:   "list",
-		Short: "List roles",
-		Long:  `List roles`,
+		Short: "List CSM roles",
+		Long:  `List CSM roles`,
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()

diff --git a/cmd/karavictl/cmd/role_update.go b/cmd/karavictl/cmd/role_update.go
@@ -30,8 +30,8 @@ import (
 func NewRoleUpdateCmd() *cobra.Command {
 	roleUpdateCmd := &cobra.Command{
 		Use:   "update",
-		Short: "Update one or more Karavi roles",
-		Long:  `Updates one or more Karavi roles`,
+		Short: "Update the quota of one or more CSM roles",
+		Long:  `Updates the quota of one or more CSM roles`,
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
@@ -86,7 +86,7 @@ func NewRoleUpdateCmd() *cobra.Command {
 
 				for _, rls := range rff.Instances() {
 					if existingRoles.Get(rls.RoleKey) == nil {
-						reportErrorAndExit(JSONOutput, cmd.ErrOrStderr(), fmt.Errorf("%s role does not exist. Try create command", rls.Name))
+						reportErrorAndExit(JSONOutput, cmd.ErrOrStderr(), fmt.Errorf(outFormat, "only role quota can be updated"))
 					}
 
 					err = validateRole(ctx, rls)

diff --git a/cmd/karavictl/cmd/role_update_test.go b/cmd/karavictl/cmd/role_update_test.go
@@ -127,6 +127,9 @@ func Test_Unit_RoleUpdate(t *testing.T) {
 				osExitCalled = true
 				done <- struct{}{}
 			}
+			defer func() {
+				osExit = os.Exit
+			}()
 
 			var err error
 			go func() {

diff --git a/internal/role-service/roles/roles.go b/internal/role-service/roles/roles.go
@@ -17,7 +17,6 @@ package roles
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"strconv"
 	"strings"
@@ -172,7 +171,7 @@ func (j *JSON) Remove(r *Instance) error {
 	defer j.mu.Unlock()
 
 	if _, ok := j.M[r.RoleKey]; !ok {
-		return errors.New("not found")
+		return fmt.Errorf("%s not found", r.String())
 	}
 	delete(j.M, r.RoleKey)
 	return nil

diff --git a/internal/role-service/service.go b/internal/role-service/service.go
@@ -263,7 +263,7 @@ func (s *Service) Update(ctx context.Context, req *pb.RoleUpdateRequest) (*pb.Ro
 	}
 
 	if existingRoles.Get(roleInstance.RoleKey) == nil {
-		return nil, fmt.Errorf("%s role does not exist. Try create command", roleInstance.Name)
+		return nil, fmt.Errorf("only role quota can be updated")
 	}
 
 	s.log.Debug("Validating role")

diff --git a/internal/role-service/service_test.go b/internal/role-service/service_test.go
@@ -386,7 +386,7 @@ func TestServiceUpdate(t *testing.T) {
 
 	// define test input
 	tests := map[string]func(t *testing.T) (*pb.RoleUpdateRequest, role.Validator, role.Kube, checkFn){
-		"success": func(t *testing.T) (*pb.RoleUpdateRequest, role.Validator, role.Kube, checkFn) {
+		"success update quota": func(t *testing.T) (*pb.RoleUpdateRequest, role.Validator, role.Kube, checkFn) {
 			req := &pb.RoleUpdateRequest{
 				Name:        "test",
 				StorageType: "powerflex",
@@ -412,6 +412,32 @@ func TestServiceUpdate(t *testing.T) {
 
 			return req, successfulValidator{}, fakeKube{GetConfiguredRolesFn: getRolesFn}, errIsNil
 		},
+		"fail update non-quota": func(t *testing.T) (*pb.RoleUpdateRequest, role.Validator, role.Kube, checkFn) {
+			req := &pb.RoleUpdateRequest{
+				Name:        "test",
+				StorageType: "powerflex",
+				SystemId:    "542a2d5f5122210f",
+				Pool:        "silver",
+				Quota:       "9GB",
+			}
+
+			ri, err := roles.NewInstance("test", "powerflex", "542a2d5f5122210f", "bronze", "9GB")
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			r := roles.NewJSON()
+			err = r.Add(ri)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			getRolesFn := func(ctx context.Context) (*roles.JSON, error) {
+				return &r, nil
+			}
+
+			return req, successfulValidator{}, fakeKube{GetConfiguredRolesFn: getRolesFn}, errIsNotNil
+		},
 		"fail validation": func(t *testing.T) (*pb.RoleUpdateRequest, role.Validator, role.Kube, checkFn) {
 			req := &pb.RoleUpdateRequest{
 				Name:        "test",