From ade9ccd517ee2885afa5ea8b22a0cda976ff5e9f Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Sun, 22 Jan 2023 02:33:43 +0000 Subject: [PATCH] Fix failing test due to the fake client indexer change in the new controller-runtime controller-runtime 0.14 includes https://github.com/kubernetes-sigs/controller-runtime/pull/2025 which turned out to be a breaking change for any tests that use fake controller-runtime client for list operations against the index. This commit addresses that, by extracting the indexer func out of the reconciler setup function so that both the reconciler setup func and the set-only fake client setup func can use the same indexer func. This fixes integration errors like the below example: ``` --- FAIL: TestWebhookWorkflowJobWithSelfHostedLabel (0.00s) --- FAIL: TestWebhookWorkflowJobWithSelfHostedLabel/Successful (0.00s) horizontal_runner_autoscaler_webhook_test.go:256: status: 500 horizontal_runner_autoscaler_webhook_test.go:256: body: List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler horizontal_runner_autoscaler_webhook_test.go:440: diagnostics: testlog] finding repository-wide runnerrepository.name=MYREPO repository.owner.login=MYORG repository.owner.type=Organization action=queued delivery= workflowJob.labels=[self-hosted label1] workflowJob.status=queued enterprise.slug= workflowJob.runID=1234567890 workflowJob.ID=1234567890 event=workflow_job hookID= repository=MYORG/MYREPO error=List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler testlog] handling check_run eventevent=workflow_job hookID= workflowJob.status=queued enterprise.slug= workflowJob.runID=1234567890 workflowJob.ID=1234567890 delivery= workflowJob.labels=[self-hosted label1] repository.name=MYREPO repository.owner.login=MYORG repository.owner.type=Organization action=queued error=List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler ``` Signed-off-by: Yusuke Kuoka --- .../horizontal_runner_autoscaler_webhook.go | 122 +++++++++--------- ...rizontal_runner_autoscaler_webhook_test.go | 6 +- 2 files changed, 67 insertions(+), 61 deletions(-) diff --git a/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook.go b/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook.go index e3710924d2..5f1f15007c 100644 --- a/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook.go +++ b/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook.go @@ -681,78 +681,80 @@ func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) SetupWithManager(mgr autoscaler.Recorder = mgr.GetEventRecorderFor(name) - if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &v1alpha1.HorizontalRunnerAutoscaler{}, scaleTargetKey, func(rawObj client.Object) []string { - hra := rawObj.(*v1alpha1.HorizontalRunnerAutoscaler) + if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &v1alpha1.HorizontalRunnerAutoscaler{}, scaleTargetKey, autoscaler.indexer); err != nil { + return err + } + + return ctrl.NewControllerManagedBy(mgr). + For(&v1alpha1.HorizontalRunnerAutoscaler{}). + Named(name). + Complete(autoscaler) +} + +func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) indexer(rawObj client.Object) []string { + hra := rawObj.(*v1alpha1.HorizontalRunnerAutoscaler) + + if hra.Spec.ScaleTargetRef.Name == "" { + autoscaler.Log.V(1).Info(fmt.Sprintf("scale target ref name not set for hra %s", hra.Name)) + return nil + } - if hra.Spec.ScaleTargetRef.Name == "" { - autoscaler.Log.V(1).Info(fmt.Sprintf("scale target ref name not set for hra %s", hra.Name)) + switch hra.Spec.ScaleTargetRef.Kind { + case "", "RunnerDeployment": + var rd v1alpha1.RunnerDeployment + if err := autoscaler.Client.Get(context.Background(), types.NamespacedName{Namespace: hra.Namespace, Name: hra.Spec.ScaleTargetRef.Name}, &rd); err != nil { + autoscaler.Log.V(1).Info(fmt.Sprintf("RunnerDeployment not found with scale target ref name %s for hra %s", hra.Spec.ScaleTargetRef.Name, hra.Name)) return nil } - switch hra.Spec.ScaleTargetRef.Kind { - case "", "RunnerDeployment": - var rd v1alpha1.RunnerDeployment - if err := autoscaler.Client.Get(context.Background(), types.NamespacedName{Namespace: hra.Namespace, Name: hra.Spec.ScaleTargetRef.Name}, &rd); err != nil { - autoscaler.Log.V(1).Info(fmt.Sprintf("RunnerDeployment not found with scale target ref name %s for hra %s", hra.Spec.ScaleTargetRef.Name, hra.Name)) - return nil - } - - keys := []string{} - if rd.Spec.Template.Spec.Repository != "" { - keys = append(keys, rd.Spec.Template.Spec.Repository) // Repository runners - } - if rd.Spec.Template.Spec.Organization != "" { - if group := rd.Spec.Template.Spec.Group; group != "" { - keys = append(keys, organizationalRunnerGroupKey(rd.Spec.Template.Spec.Organization, rd.Spec.Template.Spec.Group)) // Organization runner groups - } else { - keys = append(keys, rd.Spec.Template.Spec.Organization) // Organization runners - } - } - if enterprise := rd.Spec.Template.Spec.Enterprise; enterprise != "" { - if group := rd.Spec.Template.Spec.Group; group != "" { - keys = append(keys, enterpriseRunnerGroupKey(enterprise, rd.Spec.Template.Spec.Group)) // Enterprise runner groups - } else { - keys = append(keys, enterpriseKey(enterprise)) // Enterprise runners - } + keys := []string{} + if rd.Spec.Template.Spec.Repository != "" { + keys = append(keys, rd.Spec.Template.Spec.Repository) // Repository runners + } + if rd.Spec.Template.Spec.Organization != "" { + if group := rd.Spec.Template.Spec.Group; group != "" { + keys = append(keys, organizationalRunnerGroupKey(rd.Spec.Template.Spec.Organization, rd.Spec.Template.Spec.Group)) // Organization runner groups + } else { + keys = append(keys, rd.Spec.Template.Spec.Organization) // Organization runners } - autoscaler.Log.V(2).Info(fmt.Sprintf("HRA keys indexed for HRA %s: %v", hra.Name, keys)) - return keys - case "RunnerSet": - var rs v1alpha1.RunnerSet - if err := autoscaler.Client.Get(context.Background(), types.NamespacedName{Namespace: hra.Namespace, Name: hra.Spec.ScaleTargetRef.Name}, &rs); err != nil { - autoscaler.Log.V(1).Info(fmt.Sprintf("RunnerSet not found with scale target ref name %s for hra %s", hra.Spec.ScaleTargetRef.Name, hra.Name)) - return nil + } + if enterprise := rd.Spec.Template.Spec.Enterprise; enterprise != "" { + if group := rd.Spec.Template.Spec.Group; group != "" { + keys = append(keys, enterpriseRunnerGroupKey(enterprise, rd.Spec.Template.Spec.Group)) // Enterprise runner groups + } else { + keys = append(keys, enterpriseKey(enterprise)) // Enterprise runners } + } + autoscaler.Log.V(2).Info(fmt.Sprintf("HRA keys indexed for HRA %s: %v", hra.Name, keys)) + return keys + case "RunnerSet": + var rs v1alpha1.RunnerSet + if err := autoscaler.Client.Get(context.Background(), types.NamespacedName{Namespace: hra.Namespace, Name: hra.Spec.ScaleTargetRef.Name}, &rs); err != nil { + autoscaler.Log.V(1).Info(fmt.Sprintf("RunnerSet not found with scale target ref name %s for hra %s", hra.Spec.ScaleTargetRef.Name, hra.Name)) + return nil + } - keys := []string{} - if rs.Spec.Repository != "" { - keys = append(keys, rs.Spec.Repository) // Repository runners - } - if rs.Spec.Organization != "" { - keys = append(keys, rs.Spec.Organization) // Organization runners - if group := rs.Spec.Group; group != "" { - keys = append(keys, organizationalRunnerGroupKey(rs.Spec.Organization, rs.Spec.Group)) // Organization runner groups - } + keys := []string{} + if rs.Spec.Repository != "" { + keys = append(keys, rs.Spec.Repository) // Repository runners + } + if rs.Spec.Organization != "" { + keys = append(keys, rs.Spec.Organization) // Organization runners + if group := rs.Spec.Group; group != "" { + keys = append(keys, organizationalRunnerGroupKey(rs.Spec.Organization, rs.Spec.Group)) // Organization runner groups } - if enterprise := rs.Spec.Enterprise; enterprise != "" { - keys = append(keys, enterpriseKey(enterprise)) // Enterprise runners - if group := rs.Spec.Group; group != "" { - keys = append(keys, enterpriseRunnerGroupKey(enterprise, rs.Spec.Group)) // Enterprise runner groups - } + } + if enterprise := rs.Spec.Enterprise; enterprise != "" { + keys = append(keys, enterpriseKey(enterprise)) // Enterprise runners + if group := rs.Spec.Group; group != "" { + keys = append(keys, enterpriseRunnerGroupKey(enterprise, rs.Spec.Group)) // Enterprise runner groups } - autoscaler.Log.V(2).Info(fmt.Sprintf("HRA keys indexed for HRA %s: %v", hra.Name, keys)) - return keys } - - return nil - }); err != nil { - return err + autoscaler.Log.V(2).Info(fmt.Sprintf("HRA keys indexed for HRA %s: %v", hra.Name, keys)) + return keys } - return ctrl.NewControllerManagedBy(mgr). - For(&v1alpha1.HorizontalRunnerAutoscaler{}). - Named(name). - Complete(autoscaler) + return nil } func enterpriseKey(name string) string { diff --git a/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook_test.go b/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook_test.go index 1675b88be4..20f81f17f2 100644 --- a/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook_test.go +++ b/controllers/actions.summerwind.net/horizontal_runner_autoscaler_webhook_test.go @@ -431,7 +431,11 @@ func testServerWithInitObjs(t *testing.T, eventType string, event interface{}, w hraWebhook := &HorizontalRunnerAutoscalerGitHubWebhook{} - client := fake.NewClientBuilder().WithScheme(sc).WithRuntimeObjects(initObjs...).Build() + client := fake.NewClientBuilder(). + WithScheme(sc). + WithRuntimeObjects(initObjs...). + WithIndex(&actionsv1alpha1.HorizontalRunnerAutoscaler{}, scaleTargetKey, hraWebhook.indexer). + Build() logs := installTestLogger(hraWebhook)