From 4051f0810bd02746aa954f4e517dd2c94c2c98de Mon Sep 17 00:00:00 2001 From: Enrique Eduardo Encalada Olivas Date: Mon, 19 Feb 2024 16:00:23 +0100 Subject: [PATCH] code review feedback Signed-off-by: Sascha Schwarze --- pkg/reconciler/build/controller.go | 19 +++++++++++-- pkg/reconciler/buildrun/buildrun_test.go | 2 +- .../buildrun/resources/results_test.go | 8 +++--- .../buildrun/resources/taskrun_test.go | 25 +---------------- pkg/validate/sources.go | 6 ++--- test/e2e/v1beta1/common_suite_test.go | 1 - test/e2e/v1beta1/common_test.go | 27 +++++-------------- test/v1beta1_samples/build_samples.go | 5 ---- test/v1beta1_samples/buildstrategy_samples.go | 8 ------ 9 files changed, 32 insertions(+), 69 deletions(-) diff --git a/pkg/reconciler/build/controller.go b/pkg/reconciler/build/controller.go index ec673d9ceb..64c7aa11b7 100644 --- a/pkg/reconciler/build/controller.go +++ b/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() } } diff --git a/pkg/reconciler/buildrun/buildrun_test.go b/pkg/reconciler/buildrun/buildrun_test.go index 113b4ef77b..f05e76db6f 100644 --- a/pkg/reconciler/buildrun/buildrun_test.go +++ b/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, diff --git a/pkg/reconciler/buildrun/resources/results_test.go b/pkg/reconciler/buildrun/resources/results_test.go index 85e56098cc..26bebae104 100644 --- a/pkg/reconciler/buildrun/resources/results_test.go +++ b/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)) diff --git a/pkg/reconciler/buildrun/resources/taskrun_test.go b/pkg/reconciler/buildrun/resources/taskrun_test.go index 3c0689b634..5c3a02d976 100644 --- a/pkg/reconciler/buildrun/resources/taskrun_test.go +++ b/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()) }) diff --git a/pkg/validate/sources.go b/pkg/validate/sources.go index f33e3fe750..3b7161249c 100644 --- a/pkg/validate/sources.go +++ b/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 "": diff --git a/test/e2e/v1beta1/common_suite_test.go b/test/e2e/v1beta1/common_suite_test.go index 1be97295c3..036ddbe746 100644 --- a/test/e2e/v1beta1/common_suite_test.go +++ b/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 } diff --git a/test/e2e/v1beta1/common_test.go b/test/e2e/v1beta1/common_test.go index bfa39c97a5..fd317de76b 100644 --- a/test/e2e/v1beta1/common_test.go +++ 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 diff --git a/test/v1beta1_samples/build_samples.go b/test/v1beta1_samples/build_samples.go index 154ec55abd..271f157c1f 100644 --- a/test/v1beta1_samples/build_samples.go +++ 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 diff --git a/test/v1beta1_samples/buildstrategy_samples.go b/test/v1beta1_samples/buildstrategy_samples.go index d5d20b8225..2e11b54652 100644 --- a/test/v1beta1_samples/buildstrategy_samples.go +++ b/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