Cleanup some code smells

rollout/trafficrouting/appmesh/appmesh.go

@@ -80,7 +80,6 @@ func (r *Reconciler) UpdateHash(canaryHash, stableHash string, additionalDestina
 
 	r.log.Debugf("UpdateHash: updated stable virtual-node (%s) pod-selector to (%s)", rStableVnodeRef.Name, stableHash)
 
-	//TODO: What is the impact if stableHash is not updated but canaryHash is updated?
 	// If both hashes are same then virtual-nodes will end up with exact same podSelector. This is not allowed by the
 	// admission hook installed by appmesh controller. For now assuming that both hashes correspond to stable virtual-node
 	// when this happens and resetting canaryHash

rollout/trafficrouting/appmesh/appmesh_test.go

@@ -18,6 +18,13 @@ import (
 	k8stesting "k8s.io/client-go/testing"
 )
 
+const (
+	sampleOldCanaryHash = "canary-old"
+	sampleNewCanaryHash = "canary-new"
+	sampleOldStableHash = "stable-old"
+	sampleNewStableHash = "stable-new"
+)
+
 func fakeRollout() *v1alpha1.Rollout {
 	return &v1alpha1.Rollout{
 		ObjectMeta: metav1.ObjectMeta{
@@ -385,8 +392,8 @@ func TestSetWeightForRolloutWithRouteFilter(t *testing.T) {
 
 func TestUpdateHash(t *testing.T) {
 	type args struct {
-		inputCanaryHash    string
-		inputStableHash    string
+		newCanaryHash      string
+		newStableHash      string
 		existingCanaryHash string
 		existingStableHash string
 		expectedCanaryHash string
@@ -401,45 +408,45 @@ func TestUpdateHash(t *testing.T) {
 		{
 			name: "with no existing hashes",
 			args: args{
-				inputCanaryHash:    "canary-new",
-				expectedCanaryHash: "canary-new",
-				inputStableHash:    "stable-new",
-				expectedStableHash: "stable-new",
+				newCanaryHash:      sampleNewCanaryHash,
+				expectedCanaryHash: sampleNewCanaryHash,
+				newStableHash:      sampleNewStableHash,
+				expectedStableHash: sampleNewStableHash,
 				rollout:            fakeRollout(),
 			},
 		},
 		{
 			name: "with different existing hashes",
 			args: args{
-				inputCanaryHash:    "canary-new",
-				existingCanaryHash: "canary-old",
-				expectedCanaryHash: "canary-new",
-				inputStableHash:    "stable-new",
-				existingStableHash: "stable-old",
-				expectedStableHash: "stable-new",
+				newCanaryHash:      sampleNewCanaryHash,
+				existingCanaryHash: sampleOldCanaryHash,
+				expectedCanaryHash: sampleNewCanaryHash,
+				newStableHash:      sampleNewStableHash,
+				existingStableHash: sampleOldStableHash,
+				expectedStableHash: sampleNewStableHash,
 				rollout:            fakeRollout(),
 			},
 		},
 		{
 			name: "with existing hashes cleared",
 			args: args{
-				inputCanaryHash:    "",
-				existingCanaryHash: "canary-old",
+				newCanaryHash:      "",
+				existingCanaryHash: sampleOldCanaryHash,
 				expectedCanaryHash: defaultCanaryHash,
-				inputStableHash:    "",
-				existingStableHash: "stable-old",
+				newStableHash:      "",
+				existingStableHash: sampleOldStableHash,
 				expectedStableHash: defaultStableHash,
 				rollout:            fakeRollout(),
 			},
 		},
 		{
 			name: "with canaryHash == stableHash",
 			args: args{
-				inputCanaryHash:    "12345",
-				existingCanaryHash: "canary-old",
+				newCanaryHash:      "12345",
+				existingCanaryHash: sampleOldCanaryHash,
 				expectedCanaryHash: defaultCanaryHash,
-				existingStableHash: "stable-old",
-				inputStableHash:    "12345",
+				existingStableHash: sampleOldStableHash,
+				newStableHash:      "12345",
 				expectedStableHash: "12345",
 				rollout:            fakeRollout(),
 			},
@@ -460,7 +467,7 @@ func TestUpdateHash(t *testing.T) {
 			}
 			r := NewReconciler(cfg)
 
-			err := r.UpdateHash(fixture.args.inputCanaryHash, fixture.args.inputStableHash)
+			err := r.UpdateHash(fixture.args.newCanaryHash, fixture.args.newStableHash)
 			assert.Nil(t, err)
 			actions := client.Actions()
 			assert.Len(t, actions, 4)
@@ -475,8 +482,8 @@ func TestUpdateHash(t *testing.T) {
 }
 
 func TestUpdateHashWhenGetStableVirtualNodeFails(t *testing.T) {
-	canaryHash := "canary-new"
-	stableHash := "stable-new"
+	canaryHash := sampleNewCanaryHash
+	stableHash := sampleNewStableHash
 
 	canaryVnode := unstructuredutil.StrToUnstructuredUnsafe(baselineCanaryVnode)
 	client := testutil.NewFakeDynamicClient(canaryVnode)
@@ -495,8 +502,8 @@ func TestUpdateHashWhenGetStableVirtualNodeFails(t *testing.T) {
 }
 
 func TestUpdateHashWhenGetCanaryVirtualNodeFails(t *testing.T) {
-	canaryHash := "canary-new"
-	stableHash := "stable-new"
+	canaryHash := sampleNewCanaryHash
+	stableHash := sampleNewStableHash
 
 	stableVnode := unstructuredutil.StrToUnstructuredUnsafe(baselineStableVnode)
 	client := testutil.NewFakeDynamicClient(stableVnode)
@@ -518,8 +525,8 @@ func TestUpdateHashWhenGetCanaryVirtualNodeFails(t *testing.T) {
 }
 
 func TestUpdateHashWhenUpdateStableVirtualNodeFails(t *testing.T) {
-	canaryHash := "canary-new"
-	stableHash := "stable-new"
+	canaryHash := sampleNewCanaryHash
+	stableHash := sampleNewStableHash
 
 	canaryVnode := unstructuredutil.StrToUnstructuredUnsafe(baselineCanaryVnode)
 	stableVnode := unstructuredutil.StrToUnstructuredUnsafe(baselineStableVnode)
@@ -551,8 +558,8 @@ func TestUpdateHashWhenUpdateStableVirtualNodeFails(t *testing.T) {
 }
 
 func TestUpdateHashWhenUpdateCanaryVirtualNodeFails(t *testing.T) {
-	canaryHash := "canary-new"
-	stableHash := "stable-new"
+	canaryHash := sampleNewCanaryHash
+	stableHash := sampleNewStableHash
 
 	canaryVnode := unstructuredutil.StrToUnstructuredUnsafe(baselineCanaryVnode)
 	stableVnode := unstructuredutil.StrToUnstructuredUnsafe(baselineStableVnode)
@@ -600,8 +607,8 @@ func TestUpdateHashWithVirtualNodeMissingMatchLabels(t *testing.T) {
 	}
 	r := NewReconciler(cfg)
 
-	canaryHash := "canary-new"
-	stableHash := "stable-new"
+	canaryHash := sampleNewCanaryHash
+	stableHash := sampleNewStableHash
 	err := r.UpdateHash(canaryHash, stableHash)
 	assert.Nil(t, err)
 	actions := client.Actions()