remove status and message aggregation

kubernetes-sigs · Jun 1, 2022 · 8781492 · 8781492
1 parent 602ca98
commit 8781492
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 48 deletions.
diff --git a/internal/runtime/client/client.go b/internal/runtime/client/client.go
@@ -202,39 +202,19 @@ func (c *client) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook
 	}
 
 	// Aggregate all responses into a single object.
-	response = aggregateResponses(responses)
+	aggregateSuccessfulResponses(response, responses)
 
 	if err != nil {
 		return errors.Wrap(err, "failed to aggregate responses")
 	}
 	return nil
 }
 
-func aggregateResponses(responses map[string]runtimehooksv1.ResponseObject) runtimehooksv1.ResponseObject {
-	var aggregated runtimehooksv1.ResponseObject
-
-	for name, resp := range responses {
-		// If this is the first response use it as the aggregated response, format the message correctly and continue.
-		if aggregated == nil {
-			aggregated = resp
-			if resp.GetMessage() != "" {
-				aggregated.SetMessage(fmt.Sprintf("%s: %s", name, resp.GetMessage()))
-			}
-			continue
-		}
-
-		if resp.GetMessage() != "" {
-			// Add a new line if the aggregated message is not empty.
-			if aggregated.GetMessage() != "" {
-				aggregated.SetMessage(fmt.Sprintf("%s\n", aggregated.GetMessage()))
-			}
-			aggregated.SetMessage(fmt.Sprintf("%s%s: %s", aggregated.GetMessage(), name, resp.GetMessage()))
-		}
-		// If any single hook fails the overall Response status is Failure.
-		if resp.GetStatus() == runtimehooksv1.ResponseStatusFailure {
-			aggregated.SetStatus(runtimehooksv1.ResponseStatusFailure)
-		}
-
+func aggregateSuccessfulResponses(aggregated runtimehooksv1.ResponseObject, responses map[string]runtimehooksv1.ResponseObject) {
+	// At this point the Status should always be ResponseStatusSuccess and the Message should be empty.
+	aggregated.SetMessage("")
+	aggregated.SetStatus(runtimehooksv1.ResponseStatusSuccess)
+	for _, resp := range responses {
 		if retryable, ok := resp.(runtimehooksv1.Retryable); ok {
 			if aggregatedRetryable, aggregatedOK := aggregated.(runtimehooksv1.Retryable); aggregatedOK {
 				aggregatedRetryable.SetRetryAfterSeconds(
@@ -243,7 +223,7 @@ func aggregateResponses(responses map[string]runtimehooksv1.ResponseObject) runt
 			}
 		}
 	}
-	return aggregated
+	fmt.Print(aggregated)
 }
 
 func lowestNonZeroRetryAfterSeconds(i, j int32) int32 {

diff --git a/internal/runtime/client/client_test.go b/internal/runtime/client/client_test.go
@@ -710,60 +710,61 @@ func Test_aggregateResponses(t *testing.T) {
 		{
 			name: "Return input value with only one response",
 			responses: map[string]runtimehooksv1.ResponseObject{
-				"first": fakeResponse(runtimehooksv1.ResponseStatusFailure, "This is a failure of a hook", 5),
+				"first": fakeResponse(5),
 			},
-			want: fakeResponse(runtimehooksv1.ResponseStatusFailure, "first: This is a failure of a hook", 5),
+			want: fakeResponse(5),
 		},
 		{
 			name: "Success if no response is a failure",
 			responses: map[string]runtimehooksv1.ResponseObject{
-				"first":  fakeResponse(runtimehooksv1.ResponseStatusSuccess, "", 5),
-				"second": fakeResponse(runtimehooksv1.ResponseStatusSuccess, "", 1),
-				"third":  fakeResponse(runtimehooksv1.ResponseStatusSuccess, "", 2),
-				"fourth": fakeResponse(runtimehooksv1.ResponseStatusSuccess, "a failure", 5),
+				"first":  fakeResponse(5),
+				"second": fakeResponse(1),
+				"third":  fakeResponse(2),
+				"fourth": fakeResponse(5),
 			},
-			want: fakeResponse(runtimehooksv1.ResponseStatusSuccess, "fourth: a failure", 1),
+			want: fakeResponse(1),
 		},
 		{
 			name: "Failure if any response is a failure",
 			responses: map[string]runtimehooksv1.ResponseObject{
-				"first":  fakeResponse(runtimehooksv1.ResponseStatusSuccess, "", 5),
-				"second": fakeResponse(runtimehooksv1.ResponseStatusSuccess, "", 1),
-				"third":  fakeResponse(runtimehooksv1.ResponseStatusSuccess, "", 2),
-				"fourth": fakeResponse(runtimehooksv1.ResponseStatusFailure, "a failure", 5),
+				"first":  fakeResponse(5),
+				"second": fakeResponse(1),
+				"third":  fakeResponse(2),
+				"fourth": fakeResponse(5),
 			},
-			want: fakeResponse(runtimehooksv1.ResponseStatusFailure, "fourth: a failure", 1),
+			want: fakeResponse(1),
 		},
 
 		{
 			name: "Return lowest non-zero response for retry seconds",
 			responses: map[string]runtimehooksv1.ResponseObject{
-				"first":  fakeResponse(runtimehooksv1.ResponseStatusFailure, "This is a failure of a hook", 5),
-				"second": fakeResponse(runtimehooksv1.ResponseStatusFailure, "another failure", 1),
+				"first":  fakeResponse(5),
+				"second": fakeResponse(1),
 			},
-			want: fakeResponse(runtimehooksv1.ResponseStatusFailure, "first: This is a failure of a hook\nsecond: another failure", 1),
+			want: fakeResponse(1),
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := aggregateResponses(tt.responses)
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("aggregateResponses() got = %v, want %v", got, tt.want)
+			response := &fakev1alpha1.FakeRetryableResponse{TypeMeta: metav1.TypeMeta{Kind: "FakeResponse", APIVersion: "v1alpha1"}}
+			aggregateSuccessfulResponses(response, tt.responses)
+			if !reflect.DeepEqual(response, tt.want) {
+				t.Errorf("aggregateSuccessfulResponses() got = %v, want %v", response, tt.want)
 			}
 		})
 	}
 }
 
-func fakeResponse(status runtimehooksv1.ResponseStatus, message string, retryAfterSeconds int32) *fakev1alpha1.FakeRetryableResponse {
+func fakeResponse(retryAfterSeconds int32) *fakev1alpha1.FakeRetryableResponse {
 	return &fakev1alpha1.FakeRetryableResponse{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "FakeResponse",
 			APIVersion: "v1alpha1",
 		},
 		RetryableResponse: runtimehooksv1.RetryableResponse{
 			CommonResponse: runtimehooksv1.CommonResponse{
-				Message: message,
-				Status:  status,
+				Message: "",
+				Status:  runtimehooksv1.ResponseStatusSuccess,
 			},
 			RetryAfterSeconds: retryAfterSeconds,
 		},