PR Feedback: Re-work error reporting and tests

Author: alloveras

pkg/networking/security_group_resolver.go

@@ -34,23 +34,31 @@ type defaultSecurityGroupResolver struct {
 }
 
 func (r *defaultSecurityGroupResolver) ResolveViaNameOrID(ctx context.Context, sgNameOrIDs []string) ([]string, error) {
-	sgIDs, sgNames := r.splitIntoSgNameAndIDs(sgNameOrIDs)
 	var resolvedSGs []*ec2sdk.SecurityGroup
+	var errMessages []string
+
+	sgIDs, sgNames := r.splitIntoSgNameAndIDs(sgNameOrIDs)
 
 	if len(sgIDs) > 0 {
 		sgs, err := r.resolveViaGroupID(ctx, sgIDs)
 		if err != nil {
-			return nil, err
+			errMessages = append(errMessages, err.Error())
+		} else {
+			resolvedSGs = append(resolvedSGs, sgs...)
 		}
-		resolvedSGs = append(resolvedSGs, sgs...)
 	}
 
 	if len(sgNames) > 0 {
 		sgs, err := r.resolveViaGroupName(ctx, sgNames)
 		if err != nil {
-			return nil, err
+			errMessages = append(errMessages, err.Error())
+		} else {
+			resolvedSGs = append(resolvedSGs, sgs...)
 		}
-		resolvedSGs = append(resolvedSGs, sgs...)
+	}
+
+	if len(errMessages) > 0 {
+		return nil, errors.Errorf("couldn't find all security groups: %s", strings.Join(errMessages, ", "))
 	}
 
 	resolvedSGIDs := make([]string, 0, len(resolvedSGs))
@@ -77,11 +85,7 @@ func (r *defaultSecurityGroupResolver) resolveViaGroupID(ctx context.Context, sg
 	}
 
 	if len(sgIDs) != len(resolvedSGIDs) {
-		return nil, errors.Errorf(
-			"couldn't find all securityGroups, requested ids: [%s], found: [%s]",
-			strings.Join(sgIDs, ", "),
-			strings.Join(resolvedSGIDs, ", "),
-		)
+		return nil, errors.Errorf("requested ids [%s] but found [%s]", strings.Join(sgIDs, ", "), strings.Join(resolvedSGIDs, ", "))
 	}
 
 	return sgs, nil
@@ -120,11 +124,7 @@ func (r *defaultSecurityGroupResolver) resolveViaGroupName(ctx context.Context,
 	resolvedSGNames = algorithm.RemoveSliceDuplicates(resolvedSGNames)
 
 	if len(sgNames) != len(resolvedSGNames) {
-		return nil, errors.Errorf(
-			"couldn't find all securityGroups, requested names: [%s], found: [%s]",
-			strings.Join(sgNames, ", "),
-			strings.Join(resolvedSGNames, ", "),
-		)
+		return nil, errors.Errorf("requested names [%s] but found [%s]", strings.Join(sgNames, ", "), strings.Join(resolvedSGNames, ", "))
 	}
 
 	return sgs, nil

pkg/networking/security_group_resolver_test.go

@@ -204,7 +204,6 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
 			name: "describe by id returns error",
 			args: args{
 				nameOrIDs: []string{
-					"sg group name",
 					"sg-id",
 				},
 				describeSGCalls: []describeSecurityGroupsAsListCall{
@@ -216,24 +215,21 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
 					},
 				},
 			},
-			wantErr: errors.New("Describe.Error: unable to describe security groups"),
+			wantErr: errors.New("couldn't find all security groups: Describe.Error: unable to describe security groups"),
 		},
 		{
 			name: "describe by name returns error",
 			args: args{
 				nameOrIDs: []string{
 					"sg group name",
-					"sg-id",
 				},
 				describeSGCalls: []describeSecurityGroupsAsListCall{
 					{
 						req: &ec2sdk.DescribeSecurityGroupsInput{
 							Filters: []*ec2sdk.Filter{
 								{
-									Name: awssdk.String("tag:Name"),
-									Values: awssdk.StringSlice([]string{
-										"sg group name",
-									}),
+									Name:   awssdk.String("tag:Name"),
+									Values: awssdk.StringSlice([]string{"sg group name"}),
 								},
 								{
 									Name:   awssdk.String("vpc-id"),
@@ -243,22 +239,12 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
 						},
 						err: awserr.New("Describe.Error", "unable to describe security groups", nil),
 					},
-					{
-						req: &ec2sdk.DescribeSecurityGroupsInput{
-							GroupIds: awssdk.StringSlice([]string{"sg-id"}),
-						},
-						resp: []*ec2sdk.SecurityGroup{
-							{
-								GroupId: awssdk.String("sg-id"),
-							},
-						},
-					},
 				},
 			},
-			wantErr: errors.New("Describe.Error: unable to describe security groups"),
+			wantErr: errors.New("couldn't find all security groups: Describe.Error: unable to describe security groups"),
 		},
 		{
-			name: "unable to resolve all security group ids",
+			name: "unable to resolve security groups by id",
 			args: args{
 				nameOrIDs: []string{
 					"sg-id1",
@@ -277,10 +263,10 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
 					},
 				},
 			},
-			wantErr: errors.New("couldn't find all securityGroups, requested ids: [sg-id1, sg-id404], found: [sg-id1]"),
+			wantErr: errors.New("couldn't find all security groups: requested ids [sg-id1, sg-id404] but found [sg-id1]"),
 		},
 		{
-			name: "unable to resolve all security groups names",
+			name: "unable to resolve security groups by name",
 			args: args{
 				nameOrIDs: []string{
 					"sg group one",
@@ -314,7 +300,40 @@ func Test_defaultSecurityGroupResolver_ResolveViaNameOrID(t *testing.T) {
 					},
 				},
 			},
-			wantErr: errors.New("couldn't find all securityGroups, requested names: [sg group one, sg group two], found: [sg group one]"),
+			wantErr: errors.New("couldn't find all security groups: requested names [sg group one, sg group two] but found [sg group one]"),
+		},
+		{
+			name: "unable to resolve all security groups by ids and names",
+			args: args{
+				nameOrIDs: []string{
+					"sg-08982de7",
+					"sg group one",
+				},
+				describeSGCalls: []describeSecurityGroupsAsListCall{
+					{
+						req: &ec2sdk.DescribeSecurityGroupsInput{
+							GroupIds: awssdk.StringSlice([]string{"sg-08982de7"}),
+						},
+						resp: []*ec2sdk.SecurityGroup{},
+					},
+					{
+						req: &ec2sdk.DescribeSecurityGroupsInput{
+							Filters: []*ec2sdk.Filter{
+								{
+									Name:   awssdk.String("tag:Name"),
+									Values: awssdk.StringSlice([]string{"sg group one"}),
+								},
+								{
+									Name:   awssdk.String("vpc-id"),
+									Values: awssdk.StringSlice([]string{defaultVPCID}),
+								},
+							},
+						},
+						resp: []*ec2sdk.SecurityGroup{},
+					},
+				},
+			},
+			wantErr: errors.New("couldn't find all security groups: requested ids [sg-08982de7] but found [], requested names [sg group one] but found []"),
 		},
 	}