Apply reviewer notes

argoproj · Jun 11, 2019 · a017bd9 · a017bd9
1 parent 9f50204
commit a017bd9
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 9 deletions.
diff --git a/controller/canary.go b/controller/canary.go
@@ -19,11 +19,6 @@ import (
 func (c *Controller) rolloutCanary(rollout *v1alpha1.Rollout, rsList []*appsv1.ReplicaSet) error {
 	logCtx := logutil.WithRollout(rollout)
 
-	err := c.reconcileCanaryService(rollout)
-	if err != nil {
-		return err
-	}
-
 	if replicasetutil.CheckStepHashChange(rollout) {
 		logCtx.Info("List of Canary steps have changed and need to reset CurrentStepIndex")
 		newRS, previousRSs, err := c.getAllReplicaSetsAndSyncRevision(rollout, rsList, false)
@@ -53,6 +48,11 @@ func (c *Controller) rolloutCanary(rollout *v1alpha1.Rollout, rsList []*appsv1.R
 		allRSs = append(allRSs, stableRS)
 	}
 
+	err = c.reconcileCanaryService(rollout, newRS)
+	if err != nil {
+		return err
+	}
+
 	logCtx.Info("Reconciling StableRS")
 	scaledStableRS, err := c.reconcileStableRS(oldRSs, newRS, stableRS, rollout)
 	if err != nil {

diff --git a/controller/canary_test.go b/controller/canary_test.go
@@ -902,7 +902,7 @@ func TestCanaryRolloutStatusHPAStatusFields(t *testing.T) {
 func TestCanaryRolloutWithCanaryService(t *testing.T) {
 	f := newFixture(t)
 
-	canarySvc := newService("canary", 80, make(map[string]string))
+	canarySvc := newService("canary", 80, nil)
 	rollout := newCanaryRollout("foo", 0, nil, nil, nil, intstr.FromInt(1), intstr.FromInt(0))
 	rs := newReplicaSetWithStatus(rollout, "foo-895c6c4f9", 0, 0)
 	rollout.Spec.Strategy.CanaryStrategy.CanaryService = canarySvc.Name
@@ -912,8 +912,8 @@ func TestCanaryRolloutWithCanaryService(t *testing.T) {
 	f.kubeobjects = append(f.kubeobjects, canarySvc, rs)
 	f.serviceLister = append(f.serviceLister, canarySvc)
 
-	_ = f.expectPatchRolloutAction(rollout)
 	_ = f.expectPatchServiceAction(canarySvc, rollout.Status.CurrentPodHash)
+	_ = f.expectPatchRolloutAction(rollout)
 	f.run(getKey(rollout, t))
 }
 

diff --git a/controller/service.go b/controller/service.go
@@ -156,7 +156,7 @@ func (c *Controller) getPreviewAndActiveServices(r *v1alpha1.Rollout) (*corev1.S
 	return previewSvc, activeSvc, nil
 }
 
-func (c *Controller) reconcileCanaryService(r *v1alpha1.Rollout) error {
+func (c *Controller) reconcileCanaryService(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet) error {
 	if r.Spec.Strategy.CanaryStrategy != nil && r.Spec.Strategy.CanaryStrategy.CanaryService != "" {
 		svc, err := c.servicesLister.Services(r.Namespace).Get(r.Spec.Strategy.CanaryStrategy.CanaryService)
 		if err != nil {
@@ -175,7 +175,7 @@ func (c *Controller) reconcileCanaryService(r *v1alpha1.Rollout) error {
 			svc.Spec.Selector = make(map[string]string)
 		}
 
-		hash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount)
+		hash := controller.ComputeHash(&newRS.Spec.Template, r.Status.CollisionCount)
 		if svc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != hash {
 			patch := fmt.Sprintf(switchSelectorPatch, v1alpha1.DefaultRolloutUniqueLabelKey, hash)
 			_, err := c.kubeclientset.CoreV1().Services(svc.Namespace).Patch(svc.Name, patchtypes.StrategicMergePatchType, []byte(patch))

diff --git a/controller/service_test.go b/controller/service_test.go
@@ -260,6 +260,22 @@ func TestGetRolloutServiceKeysForCanary(t *testing.T) {
 	assert.Empty(t, keys)
 }
 
+func TestGetRolloutServiceKeysForCanaryWithCanaryService(t *testing.T) {
+	keys := getRolloutServiceKeys(&v1alpha1.Rollout{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "default",
+		},
+		Spec: v1alpha1.RolloutSpec{
+			Strategy: v1alpha1.RolloutStrategy{
+				CanaryStrategy: &v1alpha1.CanaryStrategy{
+					CanaryService: "canary-service",
+				},
+			},
+		},
+	})
+	assert.ElementsMatch(t, keys, []string{"default/canary-service"})
+}
+
 func TestGetRolloutServiceKeysForBlueGreen(t *testing.T) {
 	keys := getRolloutServiceKeys(&v1alpha1.Rollout{
 		ObjectMeta: metav1.ObjectMeta{