diff --git a/controllers/release/adapter.go b/controllers/release/adapter.go index e04d2d82a..9550f776e 100644 --- a/controllers/release/adapter.go +++ b/controllers/release/adapter.go @@ -19,6 +19,7 @@ package release import ( "context" "fmt" + "github.com/redhat-appstudio/release-service/validator" "github.com/redhat-appstudio/operator-toolkit/controller" @@ -46,17 +47,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 + validationFuncs []validator.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 +66,13 @@ func newAdapter(ctx context.Context, client client.Client, release *v1alpha1.Rel release: release, syncer: syncer.NewSyncerWithContext(client, logger, ctx), } + + releaseAdapter.validationFuncs = []validator.ValidationFunction{ + releaseAdapter.validateProcessingResources, + releaseAdapter.validateAuthor, + } + + return releaseAdapter } // EnsureFinalizersAreCalled is an operation that will ensure that finalizers are called whenever the Release being @@ -248,20 +257,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 := validator.Validate(a.validationFuncs...) + 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 +517,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() *validator.ValidationResult { if a.release.IsAttributed() { - return true, nil + return &validator.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 &validator.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 &validator.ValidationResult{Valid: false} } - return false, err + return &validator.ValidationResult{Err: err} } var author string @@ -542,31 +543,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 &validator.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 &validator.ValidationResult{Valid: false} } } a.release.Status.Attribution.Author = author - return true, nil + return &validator.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() *validator.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 &validator.ValidationResult{Valid: false} } - return false, err + return &validator.ValidationResult{Err: err} } - return true, nil + return &validator.ValidationResult{Valid: true} } diff --git a/controllers/release/adapter_test.go b/controllers/release/adapter_test.go index a201ac722..48aaa1a13 100644 --- a/controllers/release/adapter_test.go +++ b/controllers/release/adapter_test.go @@ -616,69 +616,6 @@ 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", - } - - 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 validateAuthor marks the release invalid", func() { - adapter.ctx = toolkit.GetMockedContext(ctx, []toolkit.MockData{ - { - ContextKey: loader.ReleasePlanContextKey, - 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 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, - }, - }, - }) - - 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", @@ -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/validator/validator.go b/validator/validator.go new file mode 100644 index 000000000..47988b6a2 --- /dev/null +++ b/validator/validator.go @@ -0,0 +1,19 @@ +package validator + +type ValidationFunction func() *ValidationResult + +type ValidationResult struct { + Err error + Valid bool +} + +func Validate(functions ...ValidationFunction) *ValidationResult { + for _, function := range functions { + result := function() + if !result.Valid || result.Err != nil { + return result + } + } + + return &ValidationResult{Valid: true} +}