Skip to content

Commit

Permalink
Add validator
Browse files Browse the repository at this point in the history
  • Loading branch information
davidmogar committed Sep 1, 2023
1 parent aa68f18 commit 1039526
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 131 deletions.
65 changes: 33 additions & 32 deletions controllers/release/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package release
import (
"context"
"fmt"
"github.com/redhat-appstudio/release-service/validator"

"github.com/redhat-appstudio/operator-toolkit/controller"

Expand Down Expand Up @@ -46,24 +47,32 @@ 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 []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,
logger: logger,
release: release,
syncer: syncer.NewSyncerWithContext(client, logger, ctx),
}

releaseAdapter.validations = []validator.ValidationFunction{
releaseAdapter.validateProcessingResources,
releaseAdapter.validateAuthor,
}

return releaseAdapter
}

// EnsureFinalizersAreCalled is an operation that will ensure that finalizers are called whenever the Release being
Expand Down Expand Up @@ -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.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
Expand Down Expand Up @@ -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
Expand All @@ -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}
}
135 changes: 36 additions & 99 deletions controllers/release/adapter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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"))
})
Expand All @@ -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
Expand All @@ -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"))
})
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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())
})
})

Expand Down
19 changes: 19 additions & 0 deletions validator/validator.go
Original file line number Diff line number Diff line change
@@ -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}
}

0 comments on commit 1039526

Please sign in to comment.