Move SubmarinerConfig update from submarinerbrokerinfo.Get to caller

The purpose of submarinerbrokerinfo.Get is to retrieve the broker info so it really shouldn't have the side effect of updating the passed SubmarinerConfig condition status. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
stolostron · Dec 2, 2021 · ebe5158 · ebe5158
1 parent ca368a4
commit ebe5158
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 48 deletions.
diff --git a/pkg/hub/submarineragent/controller.go b/pkg/hub/submarineragent/controller.go
@@ -426,8 +426,6 @@ func (c *submarinerAgentController) deploySubmarinerAgent(
 	brokerInfo, err := brokerinfo.Get(
 		c.kubeClient,
 		c.dynamicClient,
-		c.configClient,
-		c.eventRecorder,
 		managedCluster.Name,
 		brokerNamespace,
 		submarinerConfig,
@@ -437,6 +435,13 @@ func (c *submarinerAgentController) deploySubmarinerAgent(
 		return fmt.Errorf("failed to create submariner brokerInfo of cluster %v : %w", managedCluster.Name, err)
 	}
 
+	if submarinerConfig != nil {
+		err := c.updateSubmarinerConfigStatus(submarinerConfig, managedCluster.Name)
+		if err != nil {
+			return err
+		}
+	}
+
 	// apply submariner operator manifest work
 	operatorManifestWork, err := getManifestWork(managedCluster, brokerInfo)
 	if err != nil {
@@ -450,6 +455,26 @@ func (c *submarinerAgentController) deploySubmarinerAgent(
 	return nil
 }
 
+func (c *submarinerAgentController) updateSubmarinerConfigStatus(submarinerConfig *configv1alpha1.SubmarinerConfig,
+	clusterName string) error {
+	condition := &metav1.Condition{
+		Type:    configv1alpha1.SubmarinerConfigConditionApplied,
+		Status:  metav1.ConditionTrue,
+		Reason:  "SubmarinerConfigApplied",
+		Message: "SubmarinerConfig was applied",
+	}
+
+	_, updated, err := helpers.UpdateSubmarinerConfigStatus(c.configClient, submarinerConfig.Namespace, submarinerConfig.Name,
+		helpers.UpdateSubmarinerConfigConditionFn(condition))
+
+	if updated {
+		c.eventRecorder.Eventf("SubmarinerConfigApplied", "SubmarinerConfig %q was applied for managed cluster %q",
+			submarinerConfig.Name, clusterName)
+	}
+
+	return err
+}
+
 func (c *submarinerAgentController) removeSubmarinerAgent(ctx context.Context, clusterName string) error {
 	errs := []error{}
 	// remove submariner manifestworks

diff --git a/pkg/hub/submarineragent/controller_test.go b/pkg/hub/submarineragent/controller_test.go
@@ -88,6 +88,22 @@ var _ = Describe("Controller", func() {
 
 				It("should deploy the submariner ManifestWork with the SubmarinerConfig overrides", func() {
 					t.awaitManifestWork()
+
+					expCond := &metav1.Condition{
+						Type:   configv1alpha1.SubmarinerConfigConditionApplied,
+						Status: metav1.ConditionTrue,
+						Reason: "SubmarinerConfigApplied",
+					}
+
+					test.AwaitStatusCondition(expCond, func() ([]metav1.Condition, error) {
+						config, err := t.configClient.SubmarineraddonV1alpha1().SubmarinerConfigs(clusterName).Get(context.TODO(),
+							helpers.SubmarinerConfigName, metav1.GetOptions{})
+						if err != nil {
+							return nil, err
+						}
+
+						return config.Status.Conditions, nil
+					})
 				})
 			})
 

diff --git a/pkg/hub/submarinerbrokerinfo/brokerinfo.go b/pkg/hub/submarinerbrokerinfo/brokerinfo.go
@@ -9,10 +9,8 @@ import (
 	"strings"
 
 	configv1alpha1 "github.com/open-cluster-management/submariner-addon/pkg/apis/submarinerconfig/v1alpha1"
-	configclient "github.com/open-cluster-management/submariner-addon/pkg/client/submarinerconfig/clientset/versioned"
 	"github.com/open-cluster-management/submariner-addon/pkg/helpers"
 	apiconfigv1 "github.com/openshift/api/config/v1"
-	"github.com/openshift/library-go/pkg/operator/events"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -81,8 +79,6 @@ type SubmarinerBrokerInfo struct {
 func Get(
 	kubeClient kubernetes.Interface,
 	dynamicClient dynamic.Interface,
-	configClient configclient.Interface,
-	recorder events.Recorder,
 	clusterName string,
 	brokeNamespace string,
 	submarinerConfig *configv1alpha1.SubmarinerConfig,
@@ -124,19 +120,14 @@ func Get(
 	brokerInfo.BrokerCA = ca
 	brokerInfo.BrokerToken = token
 
-	if err := applySubmarinerConfig(configClient, recorder, brokerInfo, submarinerConfig); err != nil {
-		return nil, err
-	}
+	applySubmarinerConfig(brokerInfo, submarinerConfig)
 
 	return brokerInfo, nil
 }
 
-func applySubmarinerConfig(
-	configClient configclient.Interface,
-	recorder events.Recorder,
-	brokerInfo *SubmarinerBrokerInfo, submarinerConfig *configv1alpha1.SubmarinerConfig) error {
+func applySubmarinerConfig(brokerInfo *SubmarinerBrokerInfo, submarinerConfig *configv1alpha1.SubmarinerConfig) {
 	if submarinerConfig == nil {
-		return nil
+		return
 	}
 
 	brokerInfo.NATEnabled = submarinerConfig.Spec.NATTEnable
@@ -175,28 +166,6 @@ func applySubmarinerConfig(
 	}
 
 	applySubmarinerImageConfig(brokerInfo, submarinerConfig)
-
-	condition := metav1.Condition{
-		Type:    configv1alpha1.SubmarinerConfigConditionApplied,
-		Status:  metav1.ConditionTrue,
-		Reason:  "SubmarinerConfigApplied",
-		Message: "SubmarinerConfig was applied",
-	}
-	_, updated, err := helpers.UpdateSubmarinerConfigStatus(
-		configClient,
-		submarinerConfig.Namespace, submarinerConfig.Name,
-		helpers.UpdateSubmarinerConfigConditionFn(&condition),
-	)
-	if err != nil {
-		return err
-	}
-
-	if updated {
-		recorder.Eventf("SubmarinerConfigApplied", "SubmarinerConfig %s was applied for manged cluster %s", submarinerConfig.Name,
-			submarinerConfig.Namespace)
-	}
-
-	return nil
 }
 
 func applySubmarinerImageConfig(brokerInfo *SubmarinerBrokerInfo, submarinerConfig *configv1alpha1.SubmarinerConfig) {

diff --git a/pkg/hub/submarinerbrokerinfo/brokerinfo_test.go b/pkg/hub/submarinerbrokerinfo/brokerinfo_test.go
@@ -7,11 +7,8 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	configv1alpha1 "github.com/open-cluster-management/submariner-addon/pkg/apis/submarinerconfig/v1alpha1"
-	configclient "github.com/open-cluster-management/submariner-addon/pkg/client/submarinerconfig/clientset/versioned"
-	fakeconfigclient "github.com/open-cluster-management/submariner-addon/pkg/client/submarinerconfig/clientset/versioned/fake"
 	"github.com/open-cluster-management/submariner-addon/pkg/hub/submarinerbrokerinfo"
 	apiconfigv1 "github.com/openshift/api/config/v1"
-	"github.com/openshift/library-go/pkg/operator/events"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -38,7 +35,6 @@ func TestSubmarinerBrokerInfo(t *testing.T) {
 
 var _ = Describe("Function Get", func() {
 	var (
-		configClient          configclient.Interface
 		installationNamespace string
 		submarinerConfig      *configv1alpha1.SubmarinerConfig
 		infrastructure        *unstructured.Unstructured
@@ -102,17 +98,9 @@ var _ = Describe("Function Get", func() {
 	})
 
 	JustBeforeEach(func() {
-		if submarinerConfig != nil {
-			configClient = fakeconfigclient.NewSimpleClientset(submarinerConfig)
-		} else {
-			configClient = fakeconfigclient.NewSimpleClientset()
-		}
-
 		brokerInfo, err = submarinerbrokerinfo.Get(
 			kubefake.NewSimpleClientset(kubeObjs...),
 			dynamicfake.NewSimpleDynamicClient(runtime.NewScheme(), dynamicObjs...),
-			configClient,
-			events.NewLoggingEventRecorder("test"),
 			clusterName,
 			brokerNamespace,
 			submarinerConfig,