Onboard Secrets Manager user: addressed acceptance comments (#130)

* Addressed acceptance comments

* fix declaration in wrong place

Author: DiogoFerrao

internal/cmd/secrets-manager/user/delete/delete.go

@@ -63,13 +63,13 @@ func NewCmd() *cobra.Command {
 				instanceLabel = model.InstanceId
 			}
 
-			userLabel, userDescription, err := secretsManagerUtils.GetUserDetails(ctx, apiClient, model.ProjectId, model.InstanceId, model.UserId)
+			userLabel, err := secretsManagerUtils.GetUserLabel(ctx, apiClient, model.ProjectId, model.InstanceId, model.UserId)
 			if err != nil {
-				userLabel = model.UserId
+				userLabel = fmt.Sprintf("%q", model.UserId)
 			}
 
 			if !model.AssumeYes {
-				prompt := fmt.Sprintf("Are you sure you want to delete user %q (%q) of instance %q? (This cannot be undone)", userLabel, userDescription, instanceLabel)
+				prompt := fmt.Sprintf("Are you sure you want to delete user %s of instance %q? (This cannot be undone)", userLabel, instanceLabel)
 				err = confirm.PromptForConfirmation(cmd, prompt)
 				if err != nil {
 					return err
@@ -83,7 +83,7 @@ func NewCmd() *cobra.Command {
 				return fmt.Errorf("delete Secrets Manager user: %w", err)
 			}
 
-			cmd.Printf("Deleted user %q of instance %q\n", userLabel, instanceLabel)
+			cmd.Printf("Deleted user %s of instance %q\n", userLabel, instanceLabel)
 			return nil
 		},
 	}

internal/cmd/secrets-manager/user/update/update.go

@@ -67,13 +67,9 @@ func NewCmd() *cobra.Command {
 				instanceLabel = model.InstanceId
 			}
 
-			var userLabel string
-
-			userName, userDescription, err := secretsManagerUtils.GetUserDetails(ctx, apiClient, model.ProjectId, model.InstanceId, model.UserId)
+			userLabel, err := secretsManagerUtils.GetUserLabel(ctx, apiClient, model.ProjectId, model.InstanceId, model.UserId)
 			if err != nil {
 				userLabel = fmt.Sprintf("%q", model.UserId)
-			} else {
-				userLabel = fmt.Sprintf("%q (%s)", userName, userDescription)
 			}
 
 			if !model.AssumeYes {

internal/pkg/services/secrets-manager/utils/utils.go

@@ -20,10 +20,22 @@ func GetInstanceName(ctx context.Context, apiClient SecretsManagerClient, projec
 	return *resp.Name, nil
 }
 
-func GetUserDetails(ctx context.Context, apiClient SecretsManagerClient, projectId, instanceId, userId string) (username, description string, err error) {
+func GetUserLabel(ctx context.Context, apiClient SecretsManagerClient, projectId, instanceId, userId string) (string, error) {
 	resp, err := apiClient.GetUserExecute(ctx, projectId, instanceId, userId)
 	if err != nil {
-		return "", "", fmt.Errorf("get Secrets Manager user: %w", err)
+		return "", fmt.Errorf("get Secrets Manager user: %w", err)
 	}
-	return *resp.Username, *resp.Description, nil
+
+	if resp.Username == nil || *resp.Username == "" {
+		// Should never happen, username is auto-generated
+		return "", fmt.Errorf("username is empty")
+	}
+
+	var userLabel string
+	if resp.Description == nil || *resp.Description == "" {
+		userLabel = fmt.Sprintf("%q", *resp.Username)
+	} else {
+		userLabel = fmt.Sprintf("%q (%s)", *resp.Username, *resp.Description)
+	}
+	return userLabel, nil
 }

internal/pkg/services/secrets-manager/utils/utils_test.go

@@ -93,22 +93,51 @@ func TestGetInstanceName(t *testing.T) {
 
 func TestGetUserDetails(t *testing.T) {
 	tests := []struct {
-		description         string
-		getUserFails        bool
-		GetUserResp         *secretsmanager.User
-		isValid             bool
-		expectedUserName    string
-		expectedDescription string
+		description    string
+		getUserFails   bool
+		GetUserResp    *secretsmanager.User
+		isValid        bool
+		expectedOutput string
 	}{
 		{
 			description: "base",
 			GetUserResp: &secretsmanager.User{
 				Username:    utils.Ptr(testUserName),
 				Description: utils.Ptr(testDescription),
 			},
-			isValid:             true,
-			expectedUserName:    testUserName,
-			expectedDescription: testDescription,
+			isValid:        true,
+			expectedOutput: fmt.Sprintf("%q (%s)", testUserName, testDescription),
+		},
+		{
+			description: "user has no description",
+			GetUserResp: &secretsmanager.User{
+				Username: utils.Ptr(testUserName),
+			},
+			isValid:        true,
+			expectedOutput: fmt.Sprintf("%q", testUserName),
+		},
+		{
+			description: "user has empty description",
+			GetUserResp: &secretsmanager.User{
+				Username:    utils.Ptr(testUserName),
+				Description: utils.Ptr(""),
+			},
+			isValid:        true,
+			expectedOutput: fmt.Sprintf("%q", testUserName),
+		},
+		{
+			description: "user has empty username",
+			GetUserResp: &secretsmanager.User{
+				Username: utils.Ptr(""),
+			},
+			isValid: false,
+		},
+		{
+			description: "user has no username",
+			GetUserResp: &secretsmanager.User{
+				Username: nil,
+			},
+			isValid: false,
 		},
 		{
 			description:  "get user fails",
@@ -124,7 +153,7 @@ func TestGetUserDetails(t *testing.T) {
 				getUserResp:  tt.GetUserResp,
 			}
 
-			username, description, err := GetUserDetails(context.Background(), client, testProjectId, testInstanceId, testUserId)
+			userLabel, err := GetUserLabel(context.Background(), client, testProjectId, testInstanceId, testUserId)
 
 			if tt.isValid && err != nil {
 				t.Errorf("failed on valid input")
@@ -135,11 +164,8 @@ func TestGetUserDetails(t *testing.T) {
 			if !tt.isValid {
 				return
 			}
-			if username != tt.expectedUserName {
-				t.Errorf("expected username to be %s, got %s", tt.expectedUserName, username)
-			}
-			if description != tt.expectedDescription {
-				t.Errorf("expected description to be %s, got %s", tt.expectedDescription, description)
+			if userLabel != tt.expectedOutput {
+				t.Errorf("expected user label to be %s, got %s", tt.expectedOutput, userLabel)
 			}
 		})
 	}