From b5117a8b65a27c81d76033608a1079b471753459 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Moreno=20Garc=C3=ADa?= Date: Fri, 1 Sep 2023 15:57:50 +0200 Subject: [PATCH] feat: make use of the toolkit validator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Operator Toolkit validator allow us to define the validation functions and helps on testing the EnsureReleaseIsValid operation as now those validation functions can be mocked. Signed-off-by: David Moreno García --- controllers/release/adapter.go | 64 +++++------ controllers/release/adapter_test.go | 169 +++++++++------------------- go.mod | 2 +- go.sum | 4 +- 4 files changed, 88 insertions(+), 151 deletions(-) diff --git a/controllers/release/adapter.go b/controllers/release/adapter.go index e04d2d82..9a1cfc7f 100644 --- a/controllers/release/adapter.go +++ b/controllers/release/adapter.go @@ -46,17 +46,18 @@ import ( // adapter holds the objects needed to reconcile a Release. type adapter struct { - client client.Client - ctx context.Context - loader loader.ObjectLoader - logger *logr.Logger - release *v1alpha1.Release - syncer *syncer.Syncer + client client.Client + ctx context.Context + loader loader.ObjectLoader + logger *logr.Logger + release *v1alpha1.Release + syncer *syncer.Syncer + validations []controller.ValidationFunction } // newAdapter creates and returns an adapter instance. func newAdapter(ctx context.Context, client client.Client, release *v1alpha1.Release, loader loader.ObjectLoader, logger *logr.Logger) *adapter { - return &adapter{ + releaseAdapter := &adapter{ client: client, ctx: ctx, loader: loader, @@ -64,6 +65,13 @@ func newAdapter(ctx context.Context, client client.Client, release *v1alpha1.Rel release: release, syncer: syncer.NewSyncerWithContext(client, logger, ctx), } + + releaseAdapter.validations = []controller.ValidationFunction{ + releaseAdapter.validateProcessingResources, + releaseAdapter.validateAuthor, + } + + return releaseAdapter } // EnsureFinalizersAreCalled is an operation that will ensure that finalizers are called whenever the Release being @@ -248,20 +256,12 @@ func (a *adapter) EnsureReleaseIsProcessed() (controller.OperationResult, error) func (a *adapter) EnsureReleaseIsValid() (controller.OperationResult, error) { patch := client.MergeFrom(a.release.DeepCopy()) - validationFuncs := []func() (bool, error){ - a.validateAuthor, - a.validateProcessingResources, - } - - for _, validationFunc := range validationFuncs { - valid, err := validationFunc() - if err != nil { - return controller.RequeueWithError(err) - } - if !valid { - a.release.MarkReleaseFailed("Release validation failed") - break + result := controller.Validate(a.validations...) + if !result.Valid { + if result.Err != nil { + return controller.RequeueWithError(result.Err) } + a.release.MarkReleaseFailed("Release validation failed") } // IsReleasing will be false if MarkReleaseFailed was called @@ -516,24 +516,24 @@ func (a *adapter) syncResources() error { // validateAuthor will ensure that a valid author exists for the Release and add it to its status. If the Release // has the automated label but doesn't have automated set in its status, this function will return an error so the // operation knows to requeue the Release. -func (a *adapter) validateAuthor() (valid bool, err error) { +func (a *adapter) validateAuthor() *controller.ValidationResult { if a.release.IsAttributed() { - return true, nil + return &controller.ValidationResult{Valid: true} } if a.release.Labels[metadata.AutomatedLabel] == "true" && !a.release.IsAutomated() { err := fmt.Errorf("automated not set in status for automated release") a.release.MarkValidationFailed(err.Error()) - return false, err + return &controller.ValidationResult{Err: err} } releasePlan, err := a.loader.GetReleasePlan(a.ctx, a.client, a.release) if err != nil { if errors.IsNotFound(err) { a.release.MarkValidationFailed(err.Error()) - return false, nil + return &controller.ValidationResult{Valid: false} } - return false, err + return &controller.ValidationResult{Err: err} } var author string @@ -542,31 +542,31 @@ func (a *adapter) validateAuthor() (valid bool, err error) { author = releasePlan.Labels[metadata.AuthorLabel] if author == "" { a.release.MarkValidationFailed("no author in the ReleasePlan found for automated release") - return false, nil + return &controller.ValidationResult{Valid: false} } a.release.Status.Attribution.StandingAuthorization = true } else { author = a.release.Labels[metadata.AuthorLabel] if author == "" { // webhooks prevent this from happening but they could be disabled in some scenarios a.release.MarkValidationFailed("no author found for manual release") - return false, nil + return &controller.ValidationResult{Valid: false} } } a.release.Status.Attribution.Author = author - return true, nil + return &controller.ValidationResult{Valid: true} } // validateProcessingResources will ensure that all the resources needed to process the Release exist. -func (a *adapter) validateProcessingResources() (valid bool, err error) { +func (a *adapter) validateProcessingResources() *controller.ValidationResult { resources, err := a.loader.GetProcessingResources(a.ctx, a.client, a.release) if err != nil { if resources == nil || resources.ReleasePlanAdmission == nil || errors.IsNotFound(err) { a.release.MarkValidationFailed(err.Error()) - return false, nil + return &controller.ValidationResult{Valid: false} } - return false, err + return &controller.ValidationResult{Err: err} } - return true, nil + return &controller.ValidationResult{Valid: true} } diff --git a/controllers/release/adapter_test.go b/controllers/release/adapter_test.go index a201ac72..681d6314 100644 --- a/controllers/release/adapter_test.go +++ b/controllers/release/adapter_test.go @@ -23,6 +23,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/operator-framework/operator-lib/handler" + "github.com/redhat-appstudio/operator-toolkit/controller" toolkit "github.com/redhat-appstudio/operator-toolkit/loader" "github.com/redhat-appstudio/release-service/api/v1alpha1" "github.com/redhat-appstudio/release-service/loader" @@ -616,25 +617,22 @@ var _ = Describe("Release adapter", Ordered, func() { Expect(adapter.client.Status().Update(adapter.ctx, adapter.release)).To(Succeed()) }) - It("should requeue if validateAuthor fails with error", func() { - adapter.release.Labels = map[string]string{ - metadata.AutomatedLabel: "true", - } + It("should mark the release as validated if all checks pass", func() { + adapter.validations = []controller.ValidationFunction{} result, err := adapter.EnsureReleaseIsValid() - Expect(result.RequeueRequest && !result.CancelRequest).To(BeTrue()) - Expect(err).To(HaveOccurred()) - Expect(adapter.release.IsValid()).To(BeFalse()) + Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue()) + Expect(err).NotTo(HaveOccurred()) + Expect(adapter.release.IsValid()).To(BeTrue()) Expect(adapter.release.HasReleaseFinished()).To(BeFalse()) }) - It("should stop reconcile if validateAuthor marks the release invalid", func() { - adapter.ctx = toolkit.GetMockedContext(ctx, []toolkit.MockData{ - { - ContextKey: loader.ReleasePlanContextKey, - Err: errors.NewNotFound(schema.GroupResource{}, ""), + It("should mark the release as failed if a validation fails", func() { + adapter.validations = []controller.ValidationFunction{ + func() *controller.ValidationResult { + return &controller.ValidationResult{Valid: false} }, - }) + } result, err := adapter.EnsureReleaseIsValid() Expect(!result.RequeueRequest && result.CancelRequest).To(BeTrue()) @@ -643,82 +641,21 @@ var _ = Describe("Release adapter", Ordered, func() { Expect(adapter.release.HasReleaseFinished()).To(BeTrue()) }) - It("should requeue if validateProcessingResources fails with error", func() { - adapter.release.Labels = map[string]string{ - metadata.AuthorLabel: "user", - } - adapter.ctx = toolkit.GetMockedContext(ctx, []toolkit.MockData{ - { - ContextKey: loader.ProcessingResourcesContextKey, - Err: fmt.Errorf("internal error"), - Resource: &loader.ProcessingResources{ - ReleasePlanAdmission: releasePlanAdmission, - }, + It("should requeue the release if a validation fails with an error", func() { + adapter.validations = []controller.ValidationFunction{ + func() *controller.ValidationResult { + return &controller.ValidationResult{Err: fmt.Errorf("internal error")} }, - }) + } result, err := adapter.EnsureReleaseIsValid() Expect(result.RequeueRequest && !result.CancelRequest).To(BeTrue()) Expect(err).To(HaveOccurred()) - Expect(adapter.release.IsValid()).To(BeFalse()) - Expect(adapter.release.HasReleaseFinished()).To(BeFalse()) - }) - - It("should stop reconcile if validateProcessingResources marks the release invalid", func() { - adapter.ctx = toolkit.GetMockedContext(ctx, []toolkit.MockData{ - { - ContextKey: loader.ProcessingResourcesContextKey, - Err: errors.NewNotFound(schema.GroupResource{}, ""), - }, - }) - - result, err := adapter.EnsureReleaseIsValid() - Expect(!result.RequeueRequest && result.CancelRequest).To(BeTrue()) - Expect(err).NotTo(HaveOccurred()) - Expect(adapter.release.IsValid()).To(BeFalse()) - Expect(adapter.release.HasReleaseFinished()).To(BeTrue()) - }) - - It("should mark the release as validated if all checks pass", func() { - adapter.release.Labels = map[string]string{ - metadata.AuthorLabel: "user", - } - adapter.ctx = toolkit.GetMockedContext(ctx, []toolkit.MockData{ - { - ContextKey: loader.ProcessingResourcesContextKey, - Resource: &loader.ProcessingResources{ - EnterpriseContractConfigMap: enterpriseContractConfigMap, - EnterpriseContractPolicy: enterpriseContractPolicy, - ReleasePlanAdmission: releasePlanAdmission, - ReleaseStrategy: releaseStrategy, - Snapshot: snapshot, - }, - }, - }) - - result, err := adapter.EnsureReleaseIsValid() - Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue()) - Expect(err).NotTo(HaveOccurred()) - Expect(adapter.release.IsValid()).To(BeTrue()) Expect(adapter.release.HasReleaseFinished()).To(BeFalse()) }) It("does not clear the release status", func() { - adapter.release.Labels = map[string]string{ - metadata.AuthorLabel: "user", - } - adapter.ctx = toolkit.GetMockedContext(ctx, []toolkit.MockData{ - { - ContextKey: loader.ProcessingResourcesContextKey, - Resource: &loader.ProcessingResources{ - EnterpriseContractConfigMap: enterpriseContractConfigMap, - EnterpriseContractPolicy: enterpriseContractPolicy, - ReleasePlanAdmission: releasePlanAdmission, - ReleaseStrategy: releaseStrategy, - Snapshot: snapshot, - }, - }, - }) + adapter.validations = []controller.ValidationFunction{} result, err := adapter.EnsureReleaseIsValid() Expect(!result.RequeueRequest && !result.CancelRequest).To(BeTrue()) @@ -1286,9 +1223,9 @@ var _ = Describe("Release adapter", Ordered, func() { It("returns valid and no error if the release is already attributed", func() { adapter.release.Status.Attribution.Author = "user" - valid, err := adapter.validateAuthor() - Expect(valid).To(BeTrue()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateAuthor() + Expect(result.Valid).To(BeTrue()) + Expect(result.Err).NotTo(HaveOccurred()) }) It("should return invalid and no error if the ReleasePlan is not found", func() { @@ -1299,9 +1236,9 @@ var _ = Describe("Release adapter", Ordered, func() { }, }) - valid, err := adapter.validateAuthor() - Expect(valid).To(BeFalse()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateAuthor() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).NotTo(HaveOccurred()) }) When("the release has the automated label", func() { @@ -1312,9 +1249,9 @@ var _ = Describe("Release adapter", Ordered, func() { }) It("returns invalid and an error if automated label is present but is not set in release status", func() { - valid, err := adapter.validateAuthor() - Expect(valid).To(BeFalse()) - Expect(err).To(HaveOccurred()) + result := adapter.validateAuthor() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).To(HaveOccurred()) for i := range adapter.release.Status.Conditions { if adapter.release.Status.Conditions[i].Type == "Validated" { conditionMsg = adapter.release.Status.Conditions[i].Message @@ -1325,9 +1262,9 @@ var _ = Describe("Release adapter", Ordered, func() { It("returns invalid and an error if the ReleasePlan has no author", func() { adapter.release.Status.Automated = true - valid, err := adapter.validateAuthor() - Expect(valid).To(BeFalse()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateAuthor() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).NotTo(HaveOccurred()) for i := range adapter.release.Status.Conditions { if adapter.release.Status.Conditions[i].Type == "Validated" { conditionMsg = adapter.release.Status.Conditions[i].Message @@ -1341,9 +1278,9 @@ var _ = Describe("Release adapter", Ordered, func() { releasePlan.Labels = map[string]string{ metadata.AuthorLabel: "user", } - valid, err := adapter.validateAuthor() - Expect(valid).To(BeTrue()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateAuthor() + Expect(result.Valid).To(BeTrue()) + Expect(result.Err).NotTo(HaveOccurred()) Expect(adapter.release.Status.Attribution.StandingAuthorization).To(BeTrue()) Expect(adapter.release.Status.Attribution.Author).To(Equal("user")) }) @@ -1354,9 +1291,9 @@ var _ = Describe("Release adapter", Ordered, func() { metadata.AutomatedLabel: "false", metadata.AuthorLabel: "", } - valid, err := adapter.validateAuthor() - Expect(valid).To(BeFalse()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateAuthor() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).NotTo(HaveOccurred()) for i := range adapter.release.Status.Conditions { if adapter.release.Status.Conditions[i].Type == "Validated" { conditionMsg = adapter.release.Status.Conditions[i].Message @@ -1369,9 +1306,9 @@ var _ = Describe("Release adapter", Ordered, func() { adapter.release.Labels = map[string]string{ metadata.AuthorLabel: "user", } - valid, err := adapter.validateAuthor() - Expect(valid).To(BeTrue()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateAuthor() + Expect(result.Valid).To(BeTrue()) + Expect(result.Err).NotTo(HaveOccurred()) Expect(adapter.release.Status.Attribution.StandingAuthorization).To(BeFalse()) Expect(adapter.release.Status.Attribution.Author).To(Equal("user")) }) @@ -1403,9 +1340,9 @@ var _ = Describe("Release adapter", Ordered, func() { }, }) - valid, err := adapter.validateProcessingResources() - Expect(valid).To(BeTrue()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateProcessingResources() + Expect(result.Valid).To(BeTrue()) + Expect(result.Err).NotTo(HaveOccurred()) }) It("should return invalid and no error if any of the resources are not found", func() { @@ -1416,9 +1353,9 @@ var _ = Describe("Release adapter", Ordered, func() { }, }) - valid, err := adapter.validateProcessingResources() - Expect(valid).To(BeFalse()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateProcessingResources() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).NotTo(HaveOccurred()) }) It("should return invalid and no error if the ReleasePlanAdmission is found to be disabled", func() { @@ -1429,9 +1366,9 @@ var _ = Describe("Release adapter", Ordered, func() { }, }) - valid, err := adapter.validateProcessingResources() - Expect(valid).To(BeFalse()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateProcessingResources() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).NotTo(HaveOccurred()) }) It("should return invalid and no error if multiple ReleasePlanAdmissions exist", func() { @@ -1442,9 +1379,9 @@ var _ = Describe("Release adapter", Ordered, func() { }, }) - valid, err := adapter.validateProcessingResources() - Expect(valid).To(BeFalse()) - Expect(err).NotTo(HaveOccurred()) + result := adapter.validateProcessingResources() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).NotTo(HaveOccurred()) }) It("should return invalid and an error if some other type of error occurs", func() { @@ -1458,9 +1395,9 @@ var _ = Describe("Release adapter", Ordered, func() { }, }) - valid, err := adapter.validateProcessingResources() - Expect(valid).To(BeFalse()) - Expect(err).To(HaveOccurred()) + result := adapter.validateProcessingResources() + Expect(result.Valid).To(BeFalse()) + Expect(result.Err).To(HaveOccurred()) }) }) diff --git a/go.mod b/go.mod index fba4c4df..9330b1f4 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/onsi/gomega v1.24.1 github.com/operator-framework/operator-lib v0.10.0 github.com/redhat-appstudio/application-api v0.0.0-20230427114540-a91722251e0a - github.com/redhat-appstudio/operator-toolkit v0.0.0-20230705141436-de654b7a7aed + github.com/redhat-appstudio/operator-toolkit v0.0.0-20230901144515-eeb937692f03 github.com/tektoncd/pipeline v0.41.0 k8s.io/api v0.26.1 k8s.io/apimachinery v0.26.3 diff --git a/go.sum b/go.sum index 20b20cfb..33cc5832 100644 --- a/go.sum +++ b/go.sum @@ -306,8 +306,8 @@ github.com/redhat-appstudio/application-api v0.0.0-20230427114540-a91722251e0a/g github.com/redhat-appstudio/integration-service v0.0.0-20221130110641-f5453dea9623 h1:TRWT++4u8YPLNl825OBM6MHV1KX7OfM+kwnt/jvx+PQ= github.com/redhat-appstudio/integration-service v0.0.0-20221130110641-f5453dea9623/go.mod h1:r93vQXKEv3zFOV/8ZGs+fZTYPx2xu4CtGvlNoAO0BAY= github.com/redhat-appstudio/operator-goodies v0.0.0-20221127150647-a39dc9da8d18 h1:406ioF+WblVzI6S6SzaSrzpcE86nj9rMEJzbE0iOhHY= -github.com/redhat-appstudio/operator-toolkit v0.0.0-20230705141436-de654b7a7aed h1:4RQmIIyZPm7dZuwtPUvbTLQCNIkUR13zLIVlUE9nunA= -github.com/redhat-appstudio/operator-toolkit v0.0.0-20230705141436-de654b7a7aed/go.mod h1:6lVK58G/zkoxMoHAmYxzF6La8rMmCeZwOQk6i0dpYZo= +github.com/redhat-appstudio/operator-toolkit v0.0.0-20230901144515-eeb937692f03 h1:3cHK+a1D+VwhzGoAnC2HXbjkywT/b8jDDaiew2KgpPw= +github.com/redhat-appstudio/operator-toolkit v0.0.0-20230901144515-eeb937692f03/go.mod h1:7cX2+4KGZLJ4Yoj+1v0iV5hkCGBzbSd9wkNJQjCdDJs= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= github.com/rogpeppe/go-internal v1.8.0 h1:FCbCCtXNOY3UtUuHUYaghJg4y7Fd14rXifAYUAtL9R8=