From 37da53c6914c4db6006e2a81b814439323e4a0d2 Mon Sep 17 00:00:00 2001 From: Kiran Meduri Date: Fri, 21 Jan 2022 14:28:29 -0800 Subject: [PATCH] Cleanup some code smells --- rollout/trafficrouting/appmesh/appmesh.go | 1 - .../trafficrouting/appmesh/appmesh_test.go | 69 ++++++++++--------- 2 files changed, 38 insertions(+), 32 deletions(-) diff --git a/rollout/trafficrouting/appmesh/appmesh.go b/rollout/trafficrouting/appmesh/appmesh.go index 73db7dd3d4..d9526a9878 100644 --- a/rollout/trafficrouting/appmesh/appmesh.go +++ b/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 diff --git a/rollout/trafficrouting/appmesh/appmesh_test.go b/rollout/trafficrouting/appmesh/appmesh_test.go index 8a2a8d0579..ae37aac64d 100644 --- a/rollout/trafficrouting/appmesh/appmesh_test.go +++ b/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,33 +408,33 @@ 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(), }, @@ -435,11 +442,11 @@ func TestUpdateHash(t *testing.T) { { 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()