From f27d9e81d4ef8af39027a0607548e2dfcd6049eb Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 6 May 2022 11:44:53 +0200 Subject: [PATCH] Move predicates into package and add tests Signed-off-by: Hidde Beydals --- controllers/helmrelease_chart_controller.go | 18 ++- .../helmrelease_chart_controller_test.go | 5 +- controllers/helmrelease_controller.go | 3 +- controllers/helmrelease_controller_test.go | 5 +- .../predicates}/chart_template_predicate.go | 2 +- .../chart_template_predicate_test.go | 142 ++++++++++++++++++ .../predicates}/source_predicate.go | 16 +- internal/predicates/source_predicate_test.go | 82 ++++++++++ 8 files changed, 253 insertions(+), 20 deletions(-) rename {controllers => internal/predicates}/chart_template_predicate.go (98%) create mode 100644 internal/predicates/chart_template_predicate_test.go rename {controllers => internal/predicates}/source_predicate.go (77%) create mode 100644 internal/predicates/source_predicate_test.go diff --git a/controllers/helmrelease_chart_controller.go b/controllers/helmrelease_chart_controller.go index 5e0520665..77018d967 100644 --- a/controllers/helmrelease_chart_controller.go +++ b/controllers/helmrelease_chart_controller.go @@ -20,13 +20,6 @@ import ( "context" "fmt" - "github.com/fluxcd/helm-controller/api/v2beta1" - intcmp "github.com/fluxcd/helm-controller/internal/cmp" - "github.com/fluxcd/pkg/runtime/acl" - "github.com/fluxcd/pkg/runtime/events" - "github.com/fluxcd/pkg/runtime/patch" - "github.com/fluxcd/pkg/runtime/predicates" - sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/google/go-cmp/cmp" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -40,7 +33,16 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/ratelimiter" + "github.com/fluxcd/pkg/runtime/acl" helper "github.com/fluxcd/pkg/runtime/controller" + "github.com/fluxcd/pkg/runtime/events" + "github.com/fluxcd/pkg/runtime/patch" + "github.com/fluxcd/pkg/runtime/predicates" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + + "github.com/fluxcd/helm-controller/api/v2beta1" + intcmp "github.com/fluxcd/helm-controller/internal/cmp" + intpredicates "github.com/fluxcd/helm-controller/internal/predicates" ) type HelmReleaseChartReconciler struct { @@ -64,7 +66,7 @@ func (r *HelmReleaseChartReconciler) SetupWithManager(mgr ctrl.Manager) error { func (r *HelmReleaseChartReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, opts HelmReleaseChartReconcilerOptions) error { return ctrl.NewControllerManagedBy(mgr). For(&v2beta1.HelmRelease{}). - WithEventFilter(predicate.Or(ChartTemplateChangePredicate{}, predicates.ReconcileRequestedPredicate{})). + WithEventFilter(predicate.Or(intpredicates.ChartTemplateChangePredicate{}, predicates.ReconcileRequestedPredicate{})). WithOptions(controller.Options{ MaxConcurrentReconciles: opts.MaxConcurrentReconciles, RateLimiter: opts.RateLimiter, diff --git a/controllers/helmrelease_chart_controller_test.go b/controllers/helmrelease_chart_controller_test.go index f64c7d49f..6835c0f0a 100644 --- a/controllers/helmrelease_chart_controller_test.go +++ b/controllers/helmrelease_chart_controller_test.go @@ -21,8 +21,6 @@ import ( "testing" "time" - "github.com/fluxcd/helm-controller/api/v2beta1" - sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/go-logr/logr" . "github.com/onsi/gomega" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -34,6 +32,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/fluxcd/helm-controller/api/v2beta1" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" ) func TestHelmReleaseChartReconciler_Reconcile(t *testing.T) { diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go index af868123e..dfe982ba7 100644 --- a/controllers/helmrelease_controller.go +++ b/controllers/helmrelease_controller.go @@ -59,6 +59,7 @@ import ( intchartutil "github.com/fluxcd/helm-controller/internal/chartutil" "github.com/fluxcd/helm-controller/internal/kube" "github.com/fluxcd/helm-controller/internal/loader" + intpredicates "github.com/fluxcd/helm-controller/internal/predicates" "github.com/fluxcd/helm-controller/internal/runner" "github.com/fluxcd/helm-controller/internal/util" ) @@ -121,7 +122,7 @@ func (r *HelmReleaseReconciler) SetupWithManager(mgr ctrl.Manager, opts HelmRele Watches( &source.Kind{Type: &sourcev1.HelmChart{}}, handler.EnqueueRequestsFromMapFunc(r.requestsForHelmChartChange), - builder.WithPredicates(SourceRevisionChangePredicate{}), + builder.WithPredicates(intpredicates.SourceRevisionChangePredicate{}), ). WithOptions(controller.Options{ MaxConcurrentReconciles: opts.MaxConcurrentReconciles, diff --git a/controllers/helmrelease_controller_test.go b/controllers/helmrelease_controller_test.go index 1ec0774ad..7a1f9dab9 100644 --- a/controllers/helmrelease_controller_test.go +++ b/controllers/helmrelease_controller_test.go @@ -20,13 +20,14 @@ import ( "context" "testing" - v2 "github.com/fluxcd/helm-controller/api/v2beta1" - sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client/fake" + + v2 "github.com/fluxcd/helm-controller/api/v2beta1" + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" ) func TestHelmReleaseReconciler_getHelmChart(t *testing.T) { diff --git a/controllers/chart_template_predicate.go b/internal/predicates/chart_template_predicate.go similarity index 98% rename from controllers/chart_template_predicate.go rename to internal/predicates/chart_template_predicate.go index fad6da935..b3ed01639 100644 --- a/controllers/chart_template_predicate.go +++ b/internal/predicates/chart_template_predicate.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package controllers +package predicates import ( "github.com/fluxcd/helm-controller/api/v2beta1" diff --git a/internal/predicates/chart_template_predicate_test.go b/internal/predicates/chart_template_predicate_test.go new file mode 100644 index 000000000..67da93632 --- /dev/null +++ b/internal/predicates/chart_template_predicate_test.go @@ -0,0 +1,142 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package predicates + +import ( + "testing" + + "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + + "github.com/fluxcd/helm-controller/api/v2beta1" +) + +func TestChartTemplateChangePredicate_Create(t *testing.T) { + obj := &v2beta1.HelmRelease{Spec: v2beta1.HelmReleaseSpec{}} + suspended := &v2beta1.HelmRelease{Spec: v2beta1.HelmReleaseSpec{Suspend: true}} + not := &unstructured.Unstructured{} + + tests := []struct { + name string + obj client.Object + want bool + }{ + {name: "new", obj: obj, want: true}, + {name: "suspended", obj: suspended, want: false}, + {name: "not a HelmRelease", obj: not, want: false}, + {name: "nil", obj: nil, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := gomega.NewWithT(t) + + so := ChartTemplateChangePredicate{} + e := event.CreateEvent{ + Object: tt.obj, + } + g.Expect(so.Create(e)).To(gomega.Equal(tt.want)) + }) + } +} + +func TestChartTemplateChangePredicate_Update(t *testing.T) { + templateA := &v2beta1.HelmRelease{Spec: v2beta1.HelmReleaseSpec{ + Chart: v2beta1.HelmChartTemplate{ + Spec: v2beta1.HelmChartTemplateSpec{ + Chart: "chart-name-a", + SourceRef: v2beta1.CrossNamespaceObjectReference{ + Name: "repository", + Kind: "HelmRepository", + }, + }, + }, + }} + templateB := &v2beta1.HelmRelease{Spec: v2beta1.HelmReleaseSpec{ + Chart: v2beta1.HelmChartTemplate{ + Spec: v2beta1.HelmChartTemplateSpec{ + Chart: "chart-name-b", + SourceRef: v2beta1.CrossNamespaceObjectReference{ + Name: "repository", + Kind: "HelmRepository", + }, + }, + }, + }} + empty := &v2beta1.HelmRelease{} + suspended := &v2beta1.HelmRelease{Spec: v2beta1.HelmReleaseSpec{Suspend: true}} + not := &unstructured.Unstructured{} + + tests := []struct { + name string + old client.Object + new client.Object + want bool + }{ + {name: "same template", old: templateA, new: templateA, want: false}, + {name: "diff template", old: templateA, new: templateB, want: true}, + {name: "new with template", old: empty, new: templateA, want: true}, + {name: "old with template", old: templateA, new: empty, want: true}, + {name: "new suspended", old: templateA, new: suspended, want: false}, + {name: "old suspended new template", old: suspended, new: templateA, want: true}, + {name: "old not a HelmRelease", old: not, new: templateA, want: false}, + {name: "new not a HelmRelease", old: templateA, new: not, want: false}, + {name: "old nil", old: nil, new: templateA, want: false}, + {name: "new nil", old: templateA, new: nil, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := gomega.NewWithT(t) + + so := ChartTemplateChangePredicate{} + e := event.UpdateEvent{ + ObjectOld: tt.old, + ObjectNew: tt.new, + } + g.Expect(so.Update(e)).To(gomega.Equal(tt.want)) + }) + } +} + +func TestChartTemplateChangePredicate_Delete(t *testing.T) { + obj := &v2beta1.HelmRelease{Spec: v2beta1.HelmReleaseSpec{}} + suspended := &v2beta1.HelmRelease{Spec: v2beta1.HelmReleaseSpec{Suspend: true}} + not := &unstructured.Unstructured{} + + tests := []struct { + name string + obj client.Object + want bool + }{ + {name: "object", obj: obj, want: true}, + {name: "suspended", obj: suspended, want: false}, + {name: "not a HelmRelease", obj: not, want: false}, + {name: "nil", obj: nil, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := gomega.NewWithT(t) + + so := ChartTemplateChangePredicate{} + e := event.DeleteEvent{ + Object: tt.obj, + } + g.Expect(so.Delete(e)).To(gomega.Equal(tt.want)) + }) + } +} diff --git a/controllers/source_predicate.go b/internal/predicates/source_predicate.go similarity index 77% rename from controllers/source_predicate.go rename to internal/predicates/source_predicate.go index 60786b87e..0a97a260a 100644 --- a/controllers/source_predicate.go +++ b/internal/predicates/source_predicate.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package controllers +package predicates import ( "sigs.k8s.io/controller-runtime/pkg/event" @@ -23,6 +23,8 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" ) +// SourceRevisionChangePredicate detects revision changes to the v1beta2.Artifact +// of a v1beta2.Source object. type SourceRevisionChangePredicate struct { predicate.Funcs } @@ -42,16 +44,18 @@ func (SourceRevisionChangePredicate) Update(e event.UpdateEvent) bool { return false } - if oldSource.GetArtifact() == nil && newSource.GetArtifact() != nil { - return true + oldHasArtifact := oldSource.GetArtifact() != nil + newHasArtifact := newSource.GetArtifact() != nil + + if !newHasArtifact { + return false } - if oldSource.GetArtifact() != nil && newSource.GetArtifact() != nil && - oldSource.GetArtifact().Revision != newSource.GetArtifact().Revision { + if !oldHasArtifact { return true } - return false + return oldSource.GetArtifact().Revision != newSource.GetArtifact().Revision } func (SourceRevisionChangePredicate) Create(e event.CreateEvent) bool { diff --git a/internal/predicates/source_predicate_test.go b/internal/predicates/source_predicate_test.go new file mode 100644 index 000000000..06adda8be --- /dev/null +++ b/internal/predicates/source_predicate_test.go @@ -0,0 +1,82 @@ +/* +Copyright 2022 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package predicates + +import ( + "testing" + "time" + + "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + + sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" +) + +func TestSourceRevisionChangePredicate_Update(t *testing.T) { + sourceA := &sourceMock{revision: "revision-a"} + sourceB := &sourceMock{revision: "revision-b"} + emptySource := &sourceMock{} + notASource := &unstructured.Unstructured{} + + tests := []struct { + name string + old client.Object + new client.Object + want bool + }{ + {name: "same artifact revision", old: sourceA, new: sourceA, want: false}, + {name: "diff artifact revision", old: sourceA, new: sourceB, want: true}, + {name: "new with artifact", old: emptySource, new: sourceA, want: true}, + {name: "old with artifact", old: sourceA, new: emptySource, want: false}, + {name: "old not a source", old: notASource, new: sourceA, want: false}, + {name: "new not a source", old: sourceA, new: notASource, want: false}, + {name: "old nil", old: nil, new: sourceA, want: false}, + {name: "new nil", old: sourceA, new: nil, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := gomega.NewWithT(t) + + so := SourceRevisionChangePredicate{} + e := event.UpdateEvent{ + ObjectOld: tt.old, + ObjectNew: tt.new, + } + g.Expect(so.Update(e)).To(gomega.Equal(tt.want)) + }) + } +} + +type sourceMock struct { + unstructured.Unstructured + revision string +} + +func (m sourceMock) GetRequeueAfter() time.Duration { + return time.Second * 0 +} + +func (m *sourceMock) GetArtifact() *sourcev1.Artifact { + if m.revision != "" { + return &sourcev1.Artifact{ + Revision: m.revision, + } + } + return nil +}