From 24b40e3b7640d7dc433ec17150f241cc09d6719f Mon Sep 17 00:00:00 2001 From: Bruce Wenliang Chen Date: Thu, 13 Jun 2024 16:40:40 +0200 Subject: [PATCH] refactor: use a pointer for field skipAnalysis in CanarySpec This refactor tries to fulfil a special use case #1660: when a custom controller uses the Flagger API to render the Canary object to json/yaml, it makes sure the skipAnalysis field is rendered when the value is "false", so that the field is always communicated to the k8s API server. Comparison is as follows: before, when skipAnalysis is "false" the canary object is rendered as such: ``` apiVersion: flagger.app/v1beta1 kind: Canary ... spec: analysis: ... ``` After, it is as such: ``` apiVersion: flagger.app/v1beta1 kind: Canary ... spec: skipAnalysis: false # this is the field we expected analysis: ... ``` --- pkg/apis/flagger/v1beta1/canary.go | 9 +++++++-- pkg/controller/scheduler_daemonset_test.go | 5 +++-- pkg/controller/scheduler_deployment_test.go | 5 +++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/apis/flagger/v1beta1/canary.go b/pkg/apis/flagger/v1beta1/canary.go index 93fe004ed..e882e8ec6 100644 --- a/pkg/apis/flagger/v1beta1/canary.go +++ b/pkg/apis/flagger/v1beta1/canary.go @@ -105,7 +105,7 @@ type CanarySpec struct { // SkipAnalysis promotes the canary without analysing it // +optional - SkipAnalysis bool `json:"skipAnalysis,omitempty"` + SkipAnalysis *bool `json:"skipAnalysis,omitempty"` // revert canary mutation on deletion of canary resource // +optional @@ -615,8 +615,13 @@ func (c *Canary) GetMetricInterval() string { // SkipAnalysis returns true if the analysis is nil // or if spec.SkipAnalysis is true func (c *Canary) SkipAnalysis() bool { + if c.Spec.SkipAnalysis == nil { + return false + } + if c.Spec.Analysis == nil && c.Spec.CanaryAnalysis == nil { return true } - return c.Spec.SkipAnalysis + + return *c.Spec.SkipAnalysis } diff --git a/pkg/controller/scheduler_daemonset_test.go b/pkg/controller/scheduler_daemonset_test.go index 1340e1e64..645edbef5 100644 --- a/pkg/controller/scheduler_daemonset_test.go +++ b/pkg/controller/scheduler_daemonset_test.go @@ -110,7 +110,8 @@ func TestScheduler_DaemonSetSkipAnalysis(t *testing.T) { cd, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) require.NoError(t, err) - cd.Spec.SkipAnalysis = true + skipAnalysis := true + cd.Spec.SkipAnalysis = &skipAnalysis _, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Update(context.TODO(), cd, metav1.UpdateOptions{}) require.NoError(t, err) @@ -126,7 +127,7 @@ func TestScheduler_DaemonSetSkipAnalysis(t *testing.T) { c, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) require.NoError(t, err) - assert.True(t, c.Spec.SkipAnalysis) + assert.True(t, c.SkipAnalysis()) assert.Equal(t, flaggerv1.CanaryPhaseSucceeded, c.Status.Phase) } diff --git a/pkg/controller/scheduler_deployment_test.go b/pkg/controller/scheduler_deployment_test.go index 51161ec44..9852432f6 100644 --- a/pkg/controller/scheduler_deployment_test.go +++ b/pkg/controller/scheduler_deployment_test.go @@ -130,7 +130,8 @@ func TestScheduler_DeploymentSkipAnalysis(t *testing.T) { // enable skip cd, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) require.NoError(t, err) - cd.Spec.SkipAnalysis = true + skipAnalysis := true + cd.Spec.SkipAnalysis = &skipAnalysis _, err = mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Update(context.TODO(), cd, metav1.UpdateOptions{}) require.NoError(t, err) @@ -148,7 +149,7 @@ func TestScheduler_DeploymentSkipAnalysis(t *testing.T) { c, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), "podinfo", metav1.GetOptions{}) require.NoError(t, err) - assert.True(t, c.Spec.SkipAnalysis) + assert.True(t, c.SkipAnalysis()) assert.Equal(t, flaggerv1.CanaryPhaseSucceeded, c.Status.Phase) }