code review feedback

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

pkg/apis/build/v1beta1/buildrun_conversion.go

@@ -153,8 +153,6 @@ func (src *BuildRun) ConvertTo(ctx context.Context, obj *unstructured.Unstructur
 		CompletionTime:   src.Status.CompletionTime,
 	}
 
-	// Note: .Status.FailedAt is deprecated, so we do not
-	// convert it, only .Status.FailureDetails
 	if src.Status.FailureDetails != nil {
 		alphaBuildRun.Status.FailureDetails = &v1alpha1.FailureDetails{
 			Reason:  src.Status.FailureDetails.Reason,

pkg/reconciler/build/controller.go

@@ -83,11 +83,20 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler, maxCo
 			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 o.Spec.Retention.AtBuildDeletion != nil && *o.Spec.Retention.AtBuildDeletion &&
+						(n.Spec.Retention.AtBuildDeletion == nil || !*n.Spec.Retention.AtBuildDeletion) {
+						logAndEnableDeletion()
+					}
+					if n.Spec.Retention.AtBuildDeletion != nil && *n.Spec.Retention.AtBuildDeletion &&
+						(o.Spec.Retention.AtBuildDeletion == nil || !*o.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/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