Fix comparing parameter values on Instance Spec updates (#348)

Summary:
previously we would only recognize Instance Spec parameter updates and fail to detect an update if a parameter would be added or removed. This is fixed now.

Issue: #216

pkg/controller/framework/framework_controller.go

@@ -81,9 +81,9 @@ type ReconcileFramework struct {
 // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=kudo.k8s.io,resources=frameworks,verbs=get;list;watch;create;update;patch;delete
 func (r *ReconcileFramework) Reconcile(request reconcile.Request) (reconcile.Result, error) {
-	// Fetch the Framework instance
-	instance := &kudov1alpha1.Framework{}
-	err := r.Get(context.TODO(), request.NamespacedName, instance)
+	// Fetch the framework
+	framework := &kudov1alpha1.Framework{}
+	err := r.Get(context.TODO(), request.NamespacedName, framework)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			// Object not found, return.  Created objects are automatically garbage collected.

pkg/controller/frameworkversion/frameworkversion_controller.go

@@ -74,9 +74,9 @@ type ReconcileFrameworkVersion struct {
 // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=kudo.k8s.io,resources=frameworkversions,verbs=get;list;watch;create;update;patch;delete
 func (r *ReconcileFrameworkVersion) Reconcile(request reconcile.Request) (reconcile.Result, error) {
-	// Fetch the FrameworkVersion instance
-	instance := &kudov1alpha1.FrameworkVersion{}
-	err := r.Get(context.TODO(), request.NamespacedName, instance)
+	// Fetch the framework version
+	frameworkVersion := &kudov1alpha1.FrameworkVersion{}
+	err := r.Get(context.TODO(), request.NamespacedName, frameworkVersion)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			// Object not found, return.  Created objects are automatically garbage collected.
@@ -87,7 +87,7 @@ func (r *ReconcileFrameworkVersion) Reconcile(request reconcile.Request) (reconc
 		return reconcile.Result{}, err
 	}
 
-	log.Printf("FrameworkVersionController: Received Reconcile request for a frameworkversion named: %v", request.Name)
+	log.Printf("FrameworkVersionController: Received Reconcile request for a frameworkVersion named: %v", request.Name)
 
 	// TODO: Validate FrameworkVersion is appropriate.
 	return reconcile.Result{}, nil

pkg/controller/instance/instance_controller.go

@@ -120,31 +120,29 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 					planName = "upgrade"
 				}
 			} else if !reflect.DeepEqual(old.Spec, new.Spec) {
-				for k, v := range new.Spec.Parameters {
-					if old.Spec.Parameters[k] != v {
-						// Find the right parameter in the FV
-						for _, param := range fv.Spec.Parameters {
-							if param.Name == k {
-								planName = param.Trigger
-								ok = true
-							}
+				for k := range parameterDifference(old.Spec.Parameters, new.Spec.Parameters) {
+					// Find the right parameter in the FV
+					for _, param := range fv.Spec.Parameters {
+						if param.Name == k {
+							planName = param.Trigger
+							ok = true
 						}
+					}
+					if !ok {
+						log.Printf("InstanceController: Instance %v updated parameter %v, but parameter not found in FrameworkVersion %v\n", new.Name, k, fv.Name)
+					} else if planName == "" {
+						_, ok = fv.Spec.Plans["update"]
 						if !ok {
-							log.Printf("InstanceController: Instance %v updated parameter %v, but parameter not found in FrameworkVersion %v\n", new.Name, k, fv.Name)
-						} else if planName == "" {
-							_, ok = fv.Spec.Plans["update"]
+							_, ok = fv.Spec.Plans["deploy"]
 							if !ok {
-								_, ok = fv.Spec.Plans["deploy"]
-								if !ok {
-									log.Println("InstanceController: Could not find any plan to use for update")
-								} else {
-									planName = "deploy"
-								}
+								log.Println("InstanceController: Could not find any plan to use for update")
 							} else {
-								planName = "update"
+								planName = "deploy"
 							}
-							log.Printf("InstanceController: Instance %v updated parameter %v, but no specified trigger.  Using default plan %v\n", new.Name, k, planName)
+						} else {
+							planName = "update"
 						}
+						log.Printf("InstanceController: Instance %v updated parameter %v, but no specified trigger.  Using default plan %v\n", new.Name, k, planName)
 					}
 				}
 				// Not currently doing anything for Dependency changes.
@@ -342,3 +340,23 @@ func (r *ReconcileInstance) Reconcile(request reconcile.Request) (reconcile.Resu
 	// Defer call from above should apply the status changes to the object
 	return reconcile.Result{}, nil
 }
+
+func parameterDifference(old, new map[string]string) map[string]string {
+	diff := make(map[string]string)
+
+	for key, val := range old {
+		// If a parameter was removed in the new spec
+		if _, ok := new[key]; !ok {
+			diff[key] = val
+		}
+	}
+
+	for key, val := range new {
+		// If new spec parameter was added or changed
+		if v, ok := old[key]; !ok || v != val {
+			diff[key] = val
+		}
+	}
+
+	return diff
+}

pkg/controller/instance/instance_controller_suite_test.go

@@ -21,6 +21,8 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/onsi/gomega"
+
 	"github.com/kudobuilder/kudo/pkg/apis"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
@@ -44,3 +46,28 @@ func TestMain(m *testing.M) {
 	t.Stop()
 	os.Exit(code)
 }
+
+func TestSpecParameterDifference(t *testing.T) {
+
+	var testParams = []struct {
+		name string
+		new  map[string]string
+		diff map[string]string
+	}{
+		{"update one value", map[string]string{"one": "11", "two": "2"}, map[string]string{"one": "11"}},
+		{"update multiple values", map[string]string{"one": "11", "two": "22"}, map[string]string{"one": "11", "two": "22"}},
+		{"add new value", map[string]string{"one": "1", "two": "2", "three": "3"}, map[string]string{"three": "3"}},
+		{"remove one value", map[string]string{"one": "1"}, map[string]string{"two": "2"}},
+		{"no difference", map[string]string{"one": "1", "two": "2"}, map[string]string{}},
+		{"empty new map", map[string]string{}, map[string]string{"one": "1", "two": "2"}},
+	}
+
+	g := gomega.NewGomegaWithT(t)
+
+	var old = map[string]string{"one": "1", "two": "2"}
+
+	for _, test := range testParams {
+		diff := parameterDifference(old, test.new)
+		g.Expect(diff).Should(gomega.Equal(test.diff), test.name)
+	}
+}