Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Nov 8, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
888d35a
add failure tests and update err message of role update
atye Nov 3, 2022
978e342
change description of update command
atye Nov 3, 2022
418efc5
remove print
atye Nov 3, 2022
aa8121f
remove test
atye Nov 3, 2022
dcc092f
fix grammar
atye Nov 4, 2022
dfb413b
update description of role commands
atye Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cmd/karavictl/cmd/role_update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 Karavi roles",
Long: `Updates the quota of one or more Karavi roles`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still refer to this as a "Karavi" role?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, updated.

Run: func(cmd *cobra.Command, args []string) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions cmd/karavictl/cmd/role_update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
3 changes: 1 addition & 2 deletions internal/role-service/roles/roles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package roles

import (
"encoding/json"
"errors"
"fmt"
"strconv"
"strings"
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion internal/role-service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
28 changes: 27 additions & 1 deletion internal/role-service/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down