Issue #91 - Ability to specify canaryService Service object to reach only canary pods #100
Conversation
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
==========================================
+ Coverage 78.97% 79.09% +0.11%
==========================================
Files 15 15
Lines 1798 1808 +10
==========================================
+ Hits 1420 1430 +10
Misses 278 278
Partials 100 100
Continue to review full report at Codecov.
|
controller/service.go
Outdated
| svc.Spec.Selector = make(map[string]string) | ||
| } | ||
|
|
||
| hash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) |
There was a problem hiding this comment.
Instead of calculating the hash from the template, we should use calculate the hash from the newRS podSpec to protect against the k8s dependency change (#88).
controller/service.go
Outdated
|
|
||
| hash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) | ||
| if svc.Spec.Selector[v1alpha1.DefaultRolloutUniqueLabelKey] != hash { | ||
| patch := fmt.Sprintf(switchSelectorPatch, v1alpha1.DefaultRolloutUniqueLabelKey, hash) |
There was a problem hiding this comment.
Can you use the c.switchServiceSelector method to patch the service?
controller/canary_test.go
Outdated
| func TestCanaryRolloutWithCanaryService(t *testing.T) { | ||
| f := newFixture(t) | ||
|
|
||
| canarySvc := newService("canary", 80, make(map[string]string)) |
There was a problem hiding this comment.
What is there a reason that you instantiate the selector to nothing? If we don't, that will allow us to also test the case where the selector is nil.
There was a problem hiding this comment.
Good point. Changed make(map[string]string) to nil.
controller/service.go
Outdated
| newStatus := r.Status.DeepCopy() | ||
| cond := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionFalse, conditions.ServiceNotFoundReason, msg) | ||
| conditions.SetRolloutCondition(newStatus, *cond) | ||
| c.persistRolloutStatus(r, newStatus, &r.Spec.Paused) |
There was a problem hiding this comment.
I'm wondering if we should abstract this logic into a separate method because we have this code repeated 3 times. Thoughts?
alexmt
force-pushed
the
91-canary-preview
branch
2 times, most recently
from
7072113 to
a017bd9
Compare
controller/service.go
Outdated
| @@ -155,6 +156,35 @@ func (c *Controller) getPreviewAndActiveServices(r *v1alpha1.Rollout) (*corev1.S | |||
| return previewSvc, activeSvc, nil | |||
| } | |||
|
|
|||
| func (c *Controller) reconcileCanaryService(r *v1alpha1.Rollout, newRS *appsv1.ReplicaSet) error { | |||
| if r.Spec.Strategy.CanaryStrategy != nil && r.Spec.Strategy.CanaryStrategy.CanaryService != "" { | |||
There was a problem hiding this comment.
Style nit, we would save a block of indentation with:
if r.Spec.Strategy.CanaryStrategy == nil || r.Spec.Strategy.CanaryStrategy.CanaryService == "" {
return nil
}
alexmt
force-pushed
the
91-canary-preview
branch
2 times, most recently
from
60d9612 to
9919549
Compare
| @@ -3,17 +3,16 @@ package controller | |||
| import ( | |||
| "fmt" | |||
|
|
|||
| "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" | |||
| "github.com/argoproj/argo-rollouts/utils/conditions" | |||
| logutil "github.com/argoproj/argo-rollouts/utils/log" | |||
There was a problem hiding this comment.
Style nix: can we move this back below the external lib imports
alexmt
force-pushed
the
91-canary-preview
branch
from
9919549 to
ad9a918
Compare
…o reach only canary pods
alexmt
force-pushed
the
91-canary-preview
branch
from
ad9a918 to
8ef477d
Compare
Closes #91