code review feedback

Signed-off-by: Sascha Schwarze <[email protected]>

pkg/reconciler/build/controller.go

@@ -12,6 +12,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -80,14 +81,28 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler, maxCo
 				buildAtBuildDeletion = true
 			}
 
+			xorBuildRetentions := func(oldDeletion, newDeletion *bool) bool {
+				if oldDeletion == nil {
+					oldDeletion = pointer.Bool(false)
+				}
+				if newDeletion == nil {
+					newDeletion = pointer.Bool(false)
+				}
+				return (*oldDeletion || *newDeletion) && !(*oldDeletion && *newDeletion)
+			}
+
 			if !reflect.DeepEqual(oldBuildRetention, newBuildRetention) {
 				switch {
 				case o.Spec.Retention == nil && n.Spec.Retention != nil:
-					if n.Spec.Retention.AtBuildDeletion != nil {
+					if n.Spec.Retention.AtBuildDeletion != nil && *n.Spec.Retention.AtBuildDeletion {
 						logAndEnableDeletion()
 					}
 				case o.Spec.Retention != nil && n.Spec.Retention == nil:
-					if o.Spec.Retention.AtBuildDeletion != nil {
+					if o.Spec.Retention.AtBuildDeletion != nil && *o.Spec.Retention.AtBuildDeletion {
+						logAndEnableDeletion()
+					}
+				case o.Spec.Retention != nil && n.Spec.Retention != nil:
+					if xorBuildRetentions(o.Spec.Retention.AtBuildDeletion, n.Spec.Retention.AtBuildDeletion) {
 						logAndEnableDeletion()
 					}
 				}

pkg/reconciler/buildrun/buildrun_test.go

@@ -1173,7 +1173,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 				)
 
 				// Ensure the BuildRun gets an ownershipReference when
-				// the buildv1beta1.AnnotationBuildRunDeletion is set to true
+				// the spec.Retention.AtBuildDeletion is set to true
 				// in the build
 				clientUpdateCalls := ctl.StubBuildUpdateOwnerReferences("Build",
 					buildName,

pkg/reconciler/buildrun/resources/results_test.go

@@ -75,7 +75,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).ToNot(BeNil())
 			Expect(br.Status.Source.Git.CommitSha).To(Equal(commitSha))
 			Expect(br.Status.Source.Git.CommitAuthor).To(Equal("foo bar"))
 		})
@@ -102,7 +102,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).ToNot(BeNil())
 			Expect(br.Status.Source.OciArtifact.Digest).To(Equal(bundleImageDigest))
 		})
 
@@ -175,8 +175,8 @@ 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).ToNot(BeNil())
+			Expect(br.Status.Source.Git).ToNot(BeNil())
 			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/taskrun_test.go

@@ -35,14 +35,12 @@ var _ = Describe("GenerateTaskrun", func() {
 		buildStrategy          *buildv1beta1.BuildStrategy
 		buildStrategyStepNames map[string]struct{}
 		buildStrategyWithEnvs  *buildv1beta1.BuildStrategy
-		builderImage           *buildv1beta1.Image
-		dockerfile, buildpacks string
+		buildpacks             string
 		ctl                    test.Catalog
 	)
 
 	BeforeEach(func() {
 		buildpacks = "buildpacks-v3"
-		dockerfile = "Dockerfile"
 	})
 
 	Describe("Generate the TaskSpec", func() {
@@ -51,11 +49,6 @@ var _ = Describe("GenerateTaskrun", func() {
 			got                  *pipelineapi.TaskSpec
 			err                  error
 		)
-		BeforeEach(func() {
-			builderImage = &buildv1beta1.Image{
-				Image: "quay.io/builder/image",
-			}
-		})
 
 		Context("when the task spec is generated", func() {
 			BeforeEach(func() {
@@ -413,9 +406,6 @@ var _ = Describe("GenerateTaskrun", func() {
 
 			namespace = "build-test"
 			contextDir = "docker-build"
-			builderImage = &buildv1beta1.Image{
-				Image: "heroku/builder:22",
-			}
 			outputPath = "image-registry.openshift-image-registry.svc:5000/example/buildpacks-app"
 			outputPathBuildRun = "image-registry.openshift-image-registry.svc:5000/example/buildpacks-app-v2"
 			serviceAccountName = buildpacks + "-serviceaccount"
@@ -508,8 +498,6 @@ var _ = Describe("GenerateTaskrun", func() {
 				paramOutputInsecureFound := false
 
 				// legacy params
-				paramBuilderImageFound := false
-				paramDockerfileFound := false
 				paramContextDirFound := false
 
 				for _, param := range params {
@@ -530,14 +518,6 @@ var _ = Describe("GenerateTaskrun", func() {
 						paramOutputInsecureFound = true
 						Expect(param.Value.StringVal).To(Equal("false"))
 
-					case "builder-image":
-						paramBuilderImageFound = true
-						Expect(param.Value.StringVal).To(Equal(builderImage.Image))
-
-					case "dockerfile":
-						paramDockerfileFound = true
-						Expect(param.Value.StringVal).To(Equal(dockerfile))
-
 					case "CONTEXT_DIR":
 						paramContextDirFound = true
 						Expect(param.Value.StringVal).To(Equal(contextDir))
@@ -551,9 +531,6 @@ var _ = Describe("GenerateTaskrun", func() {
 				Expect(paramSourceContextFound).To(BeTrue())
 				Expect(paramOutputImageFound).To(BeTrue())
 				Expect(paramOutputInsecureFound).To(BeTrue())
-
-				Expect(paramBuilderImageFound).To(BeTrue())
-				Expect(paramDockerfileFound).To(BeTrue())
 				Expect(paramContextDirFound).To(BeTrue())
 			})
 

pkg/validate/sources.go

@@ -37,15 +37,15 @@ func (s *SourceRef) validateSourceEntry(source build.Source) error {
 
 	switch source.Type {
 	case "Git":
-		if source.GitSource == nil {
+		if source.GitSource == nil || source.OCIArtifact != nil || source.LocalSource != nil {
 			return fmt.Errorf("type does not match the source")
 		}
 	case "OCI":
-		if source.OCIArtifact == nil {
+		if source.OCIArtifact == nil || source.GitSource != nil || source.LocalSource != nil {
 			return fmt.Errorf("type does not match the source")
 		}
 	case "Local":
-		if source.LocalSource == nil {
+		if source.LocalSource == nil || source.OCIArtifact != nil || source.GitSource != nil {
 			return fmt.Errorf("type does not match the source")
 		}
 	case "":

test/e2e/v1beta1/common_suite_test.go

@@ -95,7 +95,6 @@ func (b *buildPrototype) SourceBundle(image string) *buildPrototype {
 		b.build.Spec.Source.OCIArtifact = &buildv1beta1.OCIArtifact{}
 	}
 	b.build.Spec.Source.Type = buildv1beta1.OCIArtifactType
-	b.build.Spec.Source.Type = buildv1beta1.OCIArtifactType
 	b.build.Spec.Source.OCIArtifact.Image = image
 	return b
 }

test/e2e/v1beta1/common_test.go

@@ -19,7 +19,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/rand"
@@ -137,34 +136,20 @@ func amendBuild(identifier string, b *buildv1beta1.Build) {
 
 // retrieveBuildAndBuildRun will retrieve the build and buildRun
 func retrieveBuildAndBuildRun(testBuild *utils.TestBuild, namespace string, buildRunName string) (*buildv1beta1.Build, *buildv1beta1.BuildRun, error) {
-	// TODO: double check the following function content.
-	buildRun, err := testBuild.LookupBuildRun(types.NamespacedName{Name: buildRunName, Namespace: namespace})
+	var build *buildv1beta1.Build
+
+	buildrun, err := testBuild.LookupBuildRun(types.NamespacedName{Name: buildRunName, Namespace: namespace})
 	if err != nil {
 		Logf("Failed to get BuildRun %q: %s", buildRunName, err)
 		return nil, nil, err
 	}
 
-	var alphaBuild buildv1beta1.Build
-	var obj unstructured.Unstructured
-
-	buildRun.ConvertTo(testBuild.Context, &obj)
-	jsonData, err := json.Marshal(obj.Object)
-	if err != nil {
-		Logf("Failed to convert the buildRun to v1alpha1: %s", err)
-	}
-
-	var alphaBuildRun buildv1beta1.BuildRun
-	json.Unmarshal(jsonData, &alphaBuildRun)
-
-	if err := resources.GetBuildObject(testBuild.Context, testBuild.ControllerRuntimeClient, &alphaBuildRun, &alphaBuild); err != nil {
+	if err := resources.GetBuildObject(testBuild.Context, testBuild.ControllerRuntimeClient, buildrun, build); err != nil {
 		Logf("Failed to get Build from BuildRun %s: %s", buildRunName, err)
-		return nil, buildRun, err
+		return nil, buildrun, err
 	}
 
-	var betaBuild buildv1beta1.Build
-	json.Unmarshal(jsonData, &betaBuild)
-
-	return &betaBuild, buildRun, nil
+	return build, buildrun, nil
 }
 
 // printTestFailureDebugInfo will output the status of Build, BuildRun, TaskRun and Pod, also print logs of Pod

test/v1beta1_samples/build_samples.go

@@ -144,11 +144,6 @@ spec:
   strategy:
     name: buildpacks-v3
     kind: ClusterBuildStrategy
-  paramValues:
-  - name: dockerfile
-    value: Dockerfile
-  - name: builder-image
-    value: heroku/builder:22
   output:
     image: image-registry.openshift-image-registry.svc:5000/example/buildpacks-app
   timeout: 30s

test/v1beta1_samples/buildstrategy_samples.go

@@ -215,14 +215,6 @@ spec:
       volumeMounts:
         - name: varlibcontainers
           mountPath: /var/lib/containers
-  parameters:
-    - name: dockerfile
-      description: The path to the Dockerfile to be used for building the image.
-      type: string
-      default: "Dockerfile"
-    - name: builder-image
-      description: The builder image.
-      type: string
 `
 
 // BuildStrategyWithParameters is a strategy that uses a