Add changes based on code review

Multiple items, most important ones:

- Reintroduce source validation for Build
- Reintroduce test case when retention is removed
  from Build
- Fix Build Update predicate for AtBuildDeletion

pkg/reconciler/build/build.go

@@ -28,6 +28,7 @@ var validationTypes = [...]string{
 	validate.SourceURL,
 	validate.Secrets,
 	validate.Strategies,
+	validate.Source,
 	validate.BuildName,
 	validate.Envs,
 	validate.Triggers,

pkg/reconciler/build/controller.go

@@ -68,18 +68,27 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler, maxCo
 			oldBuildRetention := o.Spec.Retention
 			newBuildRetention := n.Spec.Retention
 
-			if o.Spec.Retention != nil && n.Spec.Retention != nil {
-				if !reflect.DeepEqual(oldBuildRetention, newBuildRetention) {
-					if o.Spec.Retention.AtBuildDeletion != n.Spec.Retention.AtBuildDeletion {
-						ctxlog.Debug(
-							ctx,
-							"updating predicated passed, the build retention AtBuildDeletion was modified.",
-							namespace,
-							n.GetNamespace(),
-							name,
-							n.GetName(),
-						)
-						buildAtBuildDeletion = true
+			logAndEnableDeletion := func() {
+				ctxlog.Debug(
+					ctx,
+					"updating predicated passed, the build retention AtBuildDeletion was modified.",
+					namespace,
+					n.GetNamespace(),
+					name,
+					n.GetName(),
+				)
+				buildAtBuildDeletion = true
+			}
+
+			if !reflect.DeepEqual(oldBuildRetention, newBuildRetention) {
+				switch {
+				case o.Spec.Retention == nil && n.Spec.Retention != nil:
+					if n.Spec.Retention.AtBuildDeletion != nil {
+						logAndEnableDeletion()
+					}
+				case o.Spec.Retention != nil && n.Spec.Retention == nil:
+					if o.Spec.Retention.AtBuildDeletion != nil {
+						logAndEnableDeletion()
 					}
 				}
 			}

pkg/reconciler/buildrun/buildrun.go

@@ -157,6 +157,7 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req
 						validate.NewSourceURL(r.client, build),
 						validate.NewCredentials(r.client, build),
 						validate.NewStrategies(r.client, build),
+						validate.NewSourceRef(build),
 						validate.NewBuildName(build),
 						validate.NewEnv(build),
 					)

pkg/reconciler/buildrun/buildrun_test.go

@@ -1323,7 +1323,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 					})
 				})
 
-				It("should mark BuildRun as invalid if BuildRef and BuildSpec are used", func() {
+				It("should mark BuildRun as invalid if Build name and BuildSpec are used", func() {
 					buildRunSample = &build.BuildRun{
 						ObjectMeta: metav1.ObjectMeta{
 							Name: buildRunName,
@@ -1432,6 +1432,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 							Build: build.ReferencedBuild{
 								Build: &build.BuildSpec{
 									Source: build.Source{
+										Type: build.GitType,
 										GitSource: &build.Git{
 											URL: "https://github.com/shipwright-io/sample-go.git",
 										},

pkg/reconciler/buildrun/resources/credentials.go

@@ -22,31 +22,31 @@ func ApplyCredentials(ctx context.Context, build *buildv1beta1.Build, buildRun *
 	// if output is overridden by buildrun, and if this override has credentials,
 	// it should be added to the sa
 	if buildRun.Spec.Output != nil && buildRun.Spec.Output.PushSecret != nil {
-		modified = updateServiceAccountIfSecretNotLinked(ctx, buildRun.Spec.Output.PushSecret, serviceAccount) || modified
+		modified = updateServiceAccountIfSecretNotLinked(ctx, *buildRun.Spec.Output.PushSecret, serviceAccount) || modified
 	} else {
 		// otherwise, if buildrun does not override the output credentials,
 		// we should use the ones provided by the build
 		if build.Spec.Output.PushSecret != nil {
-			modified = updateServiceAccountIfSecretNotLinked(ctx, build.Spec.Output.PushSecret, serviceAccount) || modified
+			modified = updateServiceAccountIfSecretNotLinked(ctx, *build.Spec.Output.PushSecret, serviceAccount) || modified
 		}
 	}
 
 	return modified
 }
 
-func updateServiceAccountIfSecretNotLinked(ctx context.Context, sourceSecret *string, serviceAccount *corev1.ServiceAccount) bool {
+func updateServiceAccountIfSecretNotLinked(ctx context.Context, sourceSecret string, serviceAccount *corev1.ServiceAccount) bool {
 	isSecretPresent := false
 	for _, credentialSecret := range serviceAccount.Secrets {
-		if credentialSecret.Name == *sourceSecret {
+		if credentialSecret.Name == sourceSecret {
 			isSecretPresent = true
 			break
 		}
 	}
 
 	if !isSecretPresent {
-		ctxlog.Debug(ctx, "adding secret to serviceAccount", "secret", *sourceSecret, "serviceAccount", serviceAccount.Name)
+		ctxlog.Debug(ctx, "adding secret to serviceAccount", "secret", sourceSecret, "serviceAccount", serviceAccount.Name)
 		serviceAccount.Secrets = append(serviceAccount.Secrets, corev1.ObjectReference{
-			Name: *sourceSecret,
+			Name: sourceSecret,
 		})
 		return true
 	}

pkg/reconciler/buildrun/resources/results.go

@@ -29,20 +29,18 @@ func UpdateBuildRunUsingTaskResults(
 	taskRunResult []pipelineapi.TaskRunResult,
 	request reconcile.Request,
 ) {
-	// Initializing source result
-	buildRun.Status.Source = &build.SourceResult{}
-
 	// Set source results
 	updateBuildRunStatusWithSourceResult(buildRun, taskRunResult)
 
-	// Initializing output result
-	buildRun.Status.Output = &build.Output{}
-
 	// Set output results
 	updateBuildRunStatusWithOutputResult(ctx, buildRun, taskRunResult, request)
 }
 
 func updateBuildRunStatusWithOutputResult(ctx context.Context, buildRun *build.BuildRun, taskRunResult []pipelineapi.TaskRunResult, request reconcile.Request) {
+	if buildRun.Status.Output == nil {
+		buildRun.Status.Output = &build.Output{}
+	}
+
 	for _, result := range taskRunResult {
 		switch result.Name {
 		case generateOutputResultName(imageDigestResult):

pkg/reconciler/buildrun/resources/results_test.go

@@ -176,6 +176,7 @@ var _ = Describe("TaskRun results to BuildRun", func() {
 			resources.UpdateBuildRunUsingTaskResults(ctx, br, tr.Status.Results, taskRunRequest)
 
 			Expect(br.Status.Source).ToNot(Equal(nil))
+			Expect(br.Status.Source.Git).ToNot(Equal(nil))
 			Expect(br.Status.Source.Git.CommitSha).To(Equal(commitSha))
 			Expect(br.Status.Source.Git.CommitAuthor).To(Equal("foo bar"))
 			Expect(br.Status.Output.Digest).To(Equal(imageDigest))

pkg/reconciler/buildrun/resources/service_accounts.go

@@ -88,6 +88,9 @@ func GenerateSA(ctx context.Context, client client.Client, build *buildv1beta1.B
 // DeleteServiceAccount deletes the service account of a completed BuildRun if the service account
 // was generated
 func DeleteServiceAccount(ctx context.Context, client client.Client, completedBuildRun *buildv1beta1.BuildRun) error {
+	if !IsGeneratedServiceAccountUsed(completedBuildRun) {
+		return nil
+	}
 	serviceAccount := &corev1.ServiceAccount{}
 	serviceAccount.Name = GetGeneratedServiceAccountName(completedBuildRun)
 	serviceAccount.Namespace = completedBuildRun.Namespace

pkg/reconciler/buildrun/resources/service_accounts_test.go

@@ -128,7 +128,7 @@ var _ = Describe("Operating service accounts", func() {
 		})
 	})
 
-	Context("Retrieving autogenerated service accounts when the spec.serviceAccount .generated value is used", func() {
+	Context("Retrieving autogenerated service accounts when the spec.serviceAccount .generate value is used", func() {
 
 		It("should provide a generated sa name", func() {
 			Expect(resources.GetGeneratedServiceAccountName(buildRunSample)).To(Equal(buildRunSample.Name))

pkg/reconciler/buildrun/resources/sources.go

@@ -71,8 +71,10 @@ func AmendTaskSpecWithSources(
 				sources.AppendBundleStep(cfg, taskSpec, build.Spec.Source.OCIArtifact, defaultSourceName)
 			}
 		case buildv1beta1.GitType:
-			appendSourceTimestampResult(taskSpec)
-			sources.AppendGitStep(cfg, taskSpec, *build.Spec.Source.GitSource, defaultSourceName)
+			if build.Spec.Source.GitSource != nil {
+				appendSourceTimestampResult(taskSpec)
+				sources.AppendGitStep(cfg, taskSpec, *build.Spec.Source.GitSource, defaultSourceName)
+			}
 		}
 	}
 }

pkg/reconciler/buildrun/resources/sources/bundle.go

@@ -75,6 +75,9 @@ func AppendBundleResult(buildRun *build.BuildRun, name string, results []pipelin
 	imageDigest := FindResultValue(results, name, "image-digest")
 
 	if strings.TrimSpace(imageDigest) != "" {
+		if buildRun.Status.Source == nil {
+			buildRun.Status.Source = &build.SourceResult{}
+		}
 		buildRun.Status.Source.OciArtifact = &build.OciArtifactSourceResult{
 			Digest: imageDigest,
 		}

pkg/reconciler/buildrun/resources/sources/git.go

@@ -11,6 +11,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 
 	"github.com/shipwright-io/build/pkg/apis/build/v1beta1"
+	build "github.com/shipwright-io/build/pkg/apis/build/v1beta1"
 	buildv1beta1 "github.com/shipwright-io/build/pkg/apis/build/v1beta1"
 	"github.com/shipwright-io/build/pkg/config"
 
@@ -115,6 +116,9 @@ func AppendGitResult(buildRun *buildv1beta1.BuildRun, name string, results []pip
 	branchName := FindResultValue(results, name, branchName)
 
 	if strings.TrimSpace(commitAuthor) != "" || strings.TrimSpace(commitSha) != "" || strings.TrimSpace(branchName) != "" {
+		if buildRun.Status.Source == nil {
+			buildRun.Status.Source = &build.SourceResult{}
+		}
 		buildRun.Status.Source.Git = &v1beta1.GitSourceResult{
 			CommitAuthor: commitAuthor,
 			CommitSha:    commitSha,

pkg/validate/ownerreferences.go

@@ -35,40 +35,34 @@ func (o OwnerRef) ValidatePath(ctx context.Context) error {
 		return err
 	}
 
-	if o.Build.Spec.Retention != nil && o.Build.Spec.Retention.AtBuildDeletion != nil {
-		switch *o.Build.Spec.Retention.AtBuildDeletion {
-		case true:
-			// if the buildRun does not have an ownerreference to the Build, lets add it.
-			for i := range buildRunList.Items {
-				buildRun := buildRunList.Items[i]
+	if o.Build.Spec.Retention != nil && o.Build.Spec.Retention.AtBuildDeletion != nil && *o.Build.Spec.Retention.AtBuildDeletion {
+		// if the buildRun does not have an ownerreference to the Build, lets add it.
+		for i := range buildRunList.Items {
+			buildRun := buildRunList.Items[i]
 
-				if index := o.validateBuildOwnerReference(buildRun.OwnerReferences); index == -1 {
-					if err := controllerutil.SetControllerReference(o.Build, &buildRun, o.Scheme); err != nil {
-						o.Build.Status.Reason = build.BuildReasonPtr(build.SetOwnerReferenceFailed)
-						o.Build.Status.Message = pointer.String(fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err))
-					}
-					if err = o.Client.Update(ctx, &buildRun); err != nil {
-						return err
-					}
-					ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
+			if index := o.validateBuildOwnerReference(buildRun.OwnerReferences); index == -1 {
+				if err := controllerutil.SetControllerReference(o.Build, &buildRun, o.Scheme); err != nil {
+					o.Build.Status.Reason = build.BuildReasonPtr(build.SetOwnerReferenceFailed)
+					o.Build.Status.Message = pointer.String(fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err))
 				}
+				if err = o.Client.Update(ctx, &buildRun); err != nil {
+					return err
+				}
+				ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
 			}
-		case false:
-			// if the buildRun have an ownerreference to the Build, lets remove it
-			for i := range buildRunList.Items {
-				buildRun := buildRunList.Items[i]
+		}
+	} else {
+		// if the buildRun have an ownerreference to the Build, lets remove it
+		for i := range buildRunList.Items {
+			buildRun := buildRunList.Items[i]
 
-				if index := o.validateBuildOwnerReference(buildRun.OwnerReferences); index != -1 {
-					buildRun.OwnerReferences = removeOwnerReferenceByIndex(buildRun.OwnerReferences, index)
-					if err := o.Client.Update(ctx, &buildRun); err != nil {
-						return err
-					}
-					ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
+			if index := o.validateBuildOwnerReference(buildRun.OwnerReferences); index != -1 {
+				buildRun.OwnerReferences = removeOwnerReferenceByIndex(buildRun.OwnerReferences, index)
+				if err := o.Client.Update(ctx, &buildRun); err != nil {
+					return err
 				}
+				ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, buildRun.Namespace, name, buildRun.Name)
 			}
-		default:
-			ctxlog.Info(ctx, fmt.Sprintln("the build retention AtBuildDeletion was not properly defined"), namespace, o.Build.Namespace, name, o.Build.Name)
-			return fmt.Errorf("the build retention AtBuildDeletion was not properly defined")
 		}
 	}
 	return nil

pkg/validate/secrets.go

@@ -61,7 +61,7 @@ func (s Credentials) ValidatePath(ctx context.Context) error {
 func (s Credentials) buildCredentialserences() map[string]build.BuildReason {
 	// Validate if the referenced secrets exist in the namespace
 	secretRefMap := map[string]build.BuildReason{}
-	if s.Build.Spec.Output.PushSecret != nil && *s.Build.Spec.Output.PushSecret != "" {
+	if s.Build.Spec.Output.PushSecret != nil {
 		secretRefMap[*s.Build.Spec.Output.PushSecret] = build.SpecOutputSecretRefNotFound
 	}
 

pkg/validate/sources.go

@@ -0,0 +1,60 @@
+// Copyright The Shipwright Contributors
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package validate
+
+import (
+	"context"
+	"fmt"
+
+	build "github.com/shipwright-io/build/pkg/apis/build/v1beta1"
+)
+
+// SourcesRef implements RuntimeRef interface to add validations for `build.spec.source`.
+type SourceRef struct {
+	Build *build.Build // build instance for analysis
+}
+
+// ValidatePath executes the validation routine, inspecting the `build.spec.source` path
+func (s *SourceRef) ValidatePath(_ context.Context) error {
+
+	if err := s.validateSourceEntry(s.Build.Spec.Source); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// validateSourceEntry inspect informed entry, probes all required attributes.
+func (s *SourceRef) validateSourceEntry(source build.Source) error {
+
+	// dont bail out if the Source object is empty, we preserve the old behaviour as in v1alpha1
+	if source.Type == "" && source.GitSource == nil &&
+		source.OCIArtifact == nil && source.LocalSource == nil {
+		return nil
+	}
+
+	switch source.Type {
+	case "Git":
+		if source.GitSource == nil {
+			return fmt.Errorf("type does not match the source")
+		}
+	case "OCI":
+		if source.OCIArtifact == nil {
+			return fmt.Errorf("type does not match the source")
+		}
+	case "Local":
+		if source.LocalSource == nil {
+			return fmt.Errorf("type does not match the source")
+		}
+	case "":
+		return fmt.Errorf("type definition is missing")
+	}
+	return nil
+}
+
+// NewSourcesRef instantiate a new SourcesRef passing the build object pointer along.
+func NewSourceRef(b *build.Build) *SourceRef {
+	return &SourceRef{Build: b}
+}