From af2e428715a6938dbf7dcb67b77208f8e01cd6b1 Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Tue, 18 Jun 2024 12:09:57 +0200 Subject: [PATCH] post rebase fixes Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- .../observabilityaddon_controller.go | 9 ++-- ...bilityaddon_controller_integration_test.go | 33 ++----------- .../observabilityaddon_controller_test.go | 49 +++++-------------- .../endpointmetrics/pkg/rendering/renderer.go | 2 +- .../pkg/rendering/renderer_test.go | 3 +- .../endpointmetrics/pkg/status/status_test.go | 37 +++++++------- 6 files changed, 43 insertions(+), 90 deletions(-) diff --git a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go index 269717974b..8dd3e56a95 100644 --- a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go +++ b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller.go @@ -33,6 +33,7 @@ import ( "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/microshift" "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/openshift" "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/rendering" + "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/status" "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/util" oav1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1" oav1beta2 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta2" @@ -198,8 +199,8 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R log.Error(err, "OCP prometheus service does not exist") // ACM 8509: Special case for hub/local cluster metrics collection // We do not report status for hub endpoint operator - if !isHubMetricsCollector { - if err := util.ReportStatus(ctx, r.Client, util.NotSupported, obsAddon.Name, obsAddon.Namespace); err != nil { + if !r.IsHubMetricsCollector { + if err := status.ReportStatus(ctx, r.Client, status.NotSupported, obsAddon.Name, obsAddon.Namespace); err != nil { log.Error(err, "Failed to report status") } } @@ -263,8 +264,8 @@ func (r *ObservabilityAddonReconciler) Reconcile(ctx context.Context, req ctrl.R deployer := deploying.NewDeployer(r.Client) // Ordering resources to ensure they are applied in the correct order - slices.SortFunc(toDeploy, func(a, b *unstructured.Unstructured) bool { - return (resourcePriority(a) - resourcePriority(b)) < 0 + slices.SortFunc(toDeploy, func(a, b *unstructured.Unstructured) int { + return resourcePriority(a) - resourcePriority(b) }) for _, res := range toDeploy { diff --git a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_integration_test.go b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_integration_test.go index b6806dfac1..c51b98c0d9 100644 --- a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_integration_test.go +++ b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_integration_test.go @@ -83,9 +83,9 @@ func TestIntegrationReconcileHypershift(t *testing.T) { t.Fatalf("Failed to create resources: %v", err) } - mgr, err := ctrl.NewManager(testEnv.Config, ctrl.Options{ + mgr, err := ctrl.NewManager(testEnvHub.Config, ctrl.Options{ Scheme: k8sClient.Scheme(), - Metrics: metricsserver.Options{BindAddress: "0"}, + Metrics: metricsserver.Options{BindAddress: "0"}, // Avoids port conflict with the default port 8080 }) assert.NoError(t, err) @@ -175,8 +175,8 @@ func TestIntegrationReconcileMicroshift(t *testing.T) { } mgr, err := ctrl.NewManager(testEnvSpoke.Config, ctrl.Options{ - Scheme: k8sClientSpoke.Scheme(), - MetricsBindAddress: "0", // Avoids port conflict with the default port 8080 + Scheme: k8sClientSpoke.Scheme(), + Metrics: metricsserver.Options{BindAddress: "0"}, // Avoids port conflict with the default port 8080 }) assert.NoError(t, err) @@ -285,31 +285,6 @@ func createBaseScheme() *runtime.Scheme { return scheme } -func setupManager(t *testing.T, k8sClient client.Client) { - mgr, err := ctrl.NewManager(testEnvSpoke.Config, ctrl.Options{ - Scheme: k8sClient.Scheme(), - MetricsBindAddress: "0", // Avoids port conflict with the default port 8080 - }) - assert.NoError(t, err) - - hubClientWithReload, err := util.NewReloadableHubClientWithReloadFunc(func() (client.Client, error) { - return k8sClient, nil - }) - assert.NoError(t, err) - reconciler := ObservabilityAddonReconciler{ - Client: k8sClient, - HubClient: hubClientWithReload, - } - - err = reconciler.SetupWithManager(mgr) - assert.NoError(t, err) - - go func() { - err = mgr.Start(ctrl.SetupSignalHandler()) - assert.NoError(t, err) - }() -} - func setupCommonHubResources(t *testing.T, k8sClient client.Client, ns string) { // Create resources required for the observability addon controller resourcesDeps := []client.Object{ diff --git a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_test.go b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_test.go index cad1976e31..57fdbf9544 100644 --- a/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_test.go +++ b/operators/endpointmetrics/controllers/observabilityendpoint/observabilityaddon_controller_test.go @@ -21,20 +21,17 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/scheme" - clusterv1 "open-cluster-management.io/api/cluster/v1" + kubescheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" - "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/openshift" "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/util" oashared "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/shared" oav1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1" - mcov1beta2 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta2" operatorconfig "github.com/stolostron/multicluster-observability-operator/operators/pkg/config" + addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" ) const ( @@ -143,12 +140,6 @@ func newImagesCM(ns string) *corev1.ConfigMap { func init() { os.Setenv("UNIT_TEST", "true") - s := scheme.Scheme - addonv1alpha1.AddToScheme(s) - oav1beta1.AddToScheme(s) - ocinfrav1.AddToScheme(s) - hyperv1.AddToScheme(s) - promv1.AddToScheme(s) namespace = testNamespace hubNamespace = testHubNamspace @@ -192,32 +183,16 @@ alertmanager-router-ca: | }, } - scheme := scheme.Scheme - addonv1alpha1.AddToScheme(scheme) - mcov1beta2.AddToScheme(scheme) - oav1beta1.AddToScheme(scheme) - corev1.AddToScheme(scheme) - clusterv1.AddToScheme(scheme) - ocinfrav1.AddToScheme(scheme) + s := runtime.NewScheme() + kubescheme.AddToScheme(s) + addonv1alpha1.AddToScheme(s) + oav1beta1.AddToScheme(s) + ocinfrav1.AddToScheme(s) + hyperv1.AddToScheme(s) + promv1.AddToScheme(s) - hubClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithRuntimeObjects(hubObjs...). - WithStatusSubresource( - &addonv1alpha1.ManagedClusterAddOn{}, - &mcov1beta2.MultiClusterObservability{}, - &oav1beta1.ObservabilityAddon{}, - ). - Build() - c := fake.NewClientBuilder(). - WithScheme(scheme). - WithRuntimeObjects(objs...). - WithStatusSubresource( - &addonv1alpha1.ManagedClusterAddOn{}, - &mcov1beta2.MultiClusterObservability{}, - &oav1beta1.ObservabilityAddon{}, - ). - Build() + hubClient := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(hubObjs...).Build() + c := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objs...).Build() hubClientWithReload, err := util.NewReloadableHubClientWithReloadFunc(func() (client.Client, error) { return hubClient, nil @@ -228,7 +203,7 @@ alertmanager-router-ca: | r := &ObservabilityAddonReconciler{ Client: c, HubClient: hubClientWithReload, - Scheme: scheme.Scheme, + Scheme: s, IsHubMetricsCollector: false, } diff --git a/operators/endpointmetrics/pkg/rendering/renderer.go b/operators/endpointmetrics/pkg/rendering/renderer.go index 28315c7e9c..3382d98550 100644 --- a/operators/endpointmetrics/pkg/rendering/renderer.go +++ b/operators/endpointmetrics/pkg/rendering/renderer.go @@ -272,7 +272,7 @@ func Render( return resources, nil } -func getDisabledMetrics(c runtimeclient.Client) (string, error) { +func getDisabledMetrics(ctx context.Context, c runtimeclient.Client) (string, error) { cm := &corev1.ConfigMap{} err := c.Get(ctx, types.NamespacedName{Name: operatorconfig.AllowlistConfigMapName, Namespace: namespace}, cm) diff --git a/operators/endpointmetrics/pkg/rendering/renderer_test.go b/operators/endpointmetrics/pkg/rendering/renderer_test.go index 3a0a7b9783..e1a30097f5 100644 --- a/operators/endpointmetrics/pkg/rendering/renderer_test.go +++ b/operators/endpointmetrics/pkg/rendering/renderer_test.go @@ -18,6 +18,7 @@ import ( operatorconfig "github.com/stolostron/multicluster-observability-operator/operators/pkg/config" rendererutil "github.com/stolostron/multicluster-observability-operator/operators/pkg/rendering" templatesutil "github.com/stolostron/multicluster-observability-operator/operators/pkg/rendering/templates" + "github.com/stretchr/testify/assert" ) func getAllowlistCM() *corev1.ConfigMap { @@ -57,5 +58,5 @@ func TestRender(t *testing.T) { t.Fatalf("failed to render endpoint templates: %v", err) } - printObjs(t, objs) + assert.Greater(t, len(objs), 2) } diff --git a/operators/endpointmetrics/pkg/status/status_test.go b/operators/endpointmetrics/pkg/status/status_test.go index 0081475b1e..4e70fc2852 100644 --- a/operators/endpointmetrics/pkg/status/status_test.go +++ b/operators/endpointmetrics/pkg/status/status_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/stolostron/multicluster-observability-operator/operators/endpointmetrics/pkg/status" oav1beta1 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta1" mcov1beta2 "github.com/stolostron/multicluster-observability-operator/operators/multiclusterobservability/api/v1beta2" "github.com/stretchr/testify/assert" @@ -44,15 +45,15 @@ func TestReportStatus(t *testing.T) { testCases := map[string]struct { currentConditions []oav1beta1.StatusCondition - newCondition util.ConditionReason + newCondition status.ConditionReason expects func(*testing.T, []oav1beta1.StatusCondition) }{ "new status should be appended": { currentConditions: []oav1beta1.StatusCondition{}, - newCondition: util.Deployed, + newCondition: status.Deployed, expects: func(t *testing.T, conditions []oav1beta1.StatusCondition) { assert.Len(t, conditions, 1) - assert.EqualValues(t, util.Deployed, conditions[0].Reason) + assert.EqualValues(t, status.Deployed, conditions[0].Reason) assert.Equal(t, metav1.ConditionTrue, conditions[0].Status) assert.Equal(t, "Progressing", conditions[0].Type) assert.InEpsilon(t, time.Now().Unix(), conditions[0].LastTransitionTime.Unix(), 1) @@ -62,7 +63,7 @@ func TestReportStatus(t *testing.T) { currentConditions: []oav1beta1.StatusCondition{ { Type: "Progressing", - Reason: string(util.Deployed), + Reason: string(status.Deployed), Message: "Metrics collector deployed", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{ @@ -71,7 +72,7 @@ func TestReportStatus(t *testing.T) { }, { Type: "Disabled", - Reason: string(util.Disabled), + Reason: string(status.Disabled), Message: "enableMetrics is set to False", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{ @@ -79,20 +80,20 @@ func TestReportStatus(t *testing.T) { }, }, }, - newCondition: util.Disabled, + newCondition: status.Disabled, expects: func(t *testing.T, conditions []oav1beta1.StatusCondition) { assert.Len(t, conditions, 2) found := false for _, c := range conditions { - if c.Reason == string(util.Disabled) { + if c.Reason == string(status.Disabled) { found = true - assert.EqualValues(t, util.Disabled, c.Reason) + assert.EqualValues(t, status.Disabled, c.Reason) assert.Equal(t, metav1.ConditionTrue, c.Status) assert.Equal(t, "Disabled", c.Type) assert.InEpsilon(t, time.Now().Unix(), c.LastTransitionTime.Unix(), 1) } else { // other condition should not be changed - assert.EqualValues(t, util.Deployed, c.Reason) + assert.EqualValues(t, status.Deployed, c.Reason) assert.InEpsilon(t, time.Now().Add(-time.Minute).Unix(), c.LastTransitionTime.Unix(), 1) } } @@ -103,7 +104,7 @@ func TestReportStatus(t *testing.T) { currentConditions: []oav1beta1.StatusCondition{ { Type: "Progressing", - Reason: string(util.Deployed), + Reason: string(status.Deployed), Message: "Metrics collector deployed", Status: metav1.ConditionTrue, LastTransitionTime: metav1.Time{ @@ -117,10 +118,10 @@ func TestReportStatus(t *testing.T) { }, }, }, - newCondition: util.Deployed, + newCondition: status.Deployed, expects: func(t *testing.T, conditions []oav1beta1.StatusCondition) { assert.Len(t, conditions, 2) - assert.EqualValues(t, util.Deployed, conditions[0].Reason) + assert.EqualValues(t, status.Deployed, conditions[0].Reason) assert.InEpsilon(t, time.Now().Add(-time.Minute).Unix(), conditions[0].LastTransitionTime.Unix(), 1) }, }, @@ -129,10 +130,10 @@ func TestReportStatus(t *testing.T) { {Type: "1"}, {Type: "2"}, {Type: "3"}, {Type: "4"}, {Type: "5"}, {Type: "6"}, {Type: "7"}, {Type: "8"}, {Type: "9"}, {Type: "10"}, }, - newCondition: util.Deployed, + newCondition: status.Deployed, expects: func(t *testing.T, conditions []oav1beta1.StatusCondition) { - assert.Len(t, conditions, util.MaxStatusConditionsCount) - assert.EqualValues(t, util.Deployed, conditions[len(conditions)-1].Reason) + assert.Len(t, conditions, status.MaxStatusConditionsCount) + assert.EqualValues(t, status.Deployed, conditions[len(conditions)-1].Reason) }, }, "duplicated conditions should be removed": { @@ -142,7 +143,7 @@ func TestReportStatus(t *testing.T) { {Type: "Progressing", LastTransitionTime: metav1.Time{Time: time.Now().Add(-time.Minute)}}, {Type: "Degraded", LastTransitionTime: metav1.Time{Time: time.Now().Add(-time.Minute)}}, }, - newCondition: util.Deployed, + newCondition: status.Deployed, expects: func(t *testing.T, conditions []oav1beta1.StatusCondition) { assert.Len(t, conditions, 2) for _, c := range conditions { @@ -172,7 +173,7 @@ func TestReportStatus(t *testing.T) { } // test - if err := util.ReportStatus(context.Background(), client, tc.newCondition, baseAddon.Name, baseAddon.Namespace); err != nil { + if err := status.ReportStatus(context.Background(), client, tc.newCondition, baseAddon.Name, baseAddon.Namespace); err != nil { t.Fatalf("Error reporting status: %v", err) } newAddon := &oav1beta1.ObservabilityAddon{} @@ -200,7 +201,7 @@ func TestReportStatus_Conflict(t *testing.T) { conflictErr := errors.NewConflict(schema.GroupResource{Group: oav1beta1.GroupVersion.Group, Resource: "resource"}, name, fmt.Errorf("conflict")) c := newClientWithUpdateError(fakeClient, conflictErr) - if err := util.ReportStatus(context.Background(), c, util.Deployed, name, testNamespace); err == nil { + if err := status.ReportStatus(context.Background(), c, status.Deployed, name, testNamespace); err == nil { t.Fatalf("Conflict error should be retried and return an error if it fails") } if c.UpdateCallsCount() <= 1 {