Repository: shipwright-io/build
Pull request #1381 (2023-09-14 19:07:45)

Handle dockerfile in build strategy conversion
# Changes

This change completes the handling of Dockerfiles:

* When a Beta build contains a Dockerfile parameter, then this is converted to .spec.dockerfile in Alpha without adding the parameter
* An Alpha build strategy that uses `$(build.dockerfile)` or `$(params.DOCKERFILE)` gets their usage migrated to `$(params.dockerfile)` and the dockerfile parameter is declared
* A Beta build strategy that declares a dockerfile parameter as string with Dockerfile as default value gets this parameter removed and all referenced changed to `$(params.DOCKERFILE)
`
I am also adding a commit that fixes the conversion of the retention from alpha to beta. The old code worked find in unit tests but not in reality. We must create a copy of the Map so that the annotation addition works.

# Submitter Checklist

- [x] Includes tests if functionality changed/was added
- [ ] Includes docs if changes are user-facing
- [x] [Set a kind label on this PR](https://prow.k8s.io/command-help#kind)
- [x] Release notes block has been filled in, or marked NONE

# Release Notes

```release-note
NONE
```


openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please ask for approval from [saschaschwarze0](https://github.com/saschaschwarze0). For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=shipwright-io%2Fbuild).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/shipwright-io/build/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["saschaschwarze0"]} -->

Patch:
diff --git a/hack/generate-cert.sh b/hack/generate-cert.sh
index 60ef459a..5023293c 100755
--- a/hack/generate-cert.sh
+++ b/hack/generate-cert.sh
@@ -1,9 +1,9 @@
+#!/bin/bash
+
 # Copyright The Shipwright Contributors
 #
 # SPDX-License-Identifier: Apache-2.0
 
-#!/bin/bash
-
 set -euo pipefail
 
 DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")"/.. && pwd)"
diff --git a/hack/patch-crds-with-conversion.sh b/hack/patch-crds-with-conversion.sh
index cbf05971..b1487b50 100755
--- a/hack/patch-crds-with-conversion.sh
+++ b/hack/patch-crds-with-conversion.sh
@@ -1,9 +1,9 @@
+#!/bin/bash
+
 # Copyright The Shipwright Contributors
 #
 # SPDX-License-Identifier: Apache-2.0
 
-#!/bin/bash
-
 set -euo pipefail
 
 DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")"/.. && pwd)"
diff --git a/pkg/apis/build/v1beta1/build_conversion.go b/pkg/apis/build/v1beta1/build_conversion.go
index 25e155f1..a611063c 100644
--- a/pkg/apis/build/v1beta1/build_conversion.go
+++ b/pkg/apis/build/v1beta1/build_conversion.go
@@ -27,6 +27,8 @@ var _ webhook.Conversion = (*Build)(nil)
 
 // ConvertTo converts this Build object to v1alpha1 format.
 func (src *Build) ConvertTo(ctx context.Context, obj *unstructured.Unstructured) error {
+	ctxlog.Debug(ctx, "Converting Build from beta to alpha", "namespace", src.Namespace, "name", src.Name)
+
 	var alphaBuild v1alpha1.Build
 
 	alphaBuild.TypeMeta = src.TypeMeta
@@ -38,8 +40,10 @@ func (src *Build) ConvertTo(ctx context.Context, obj *unstructured.Unstructured)
 
 	// convert annotation-controlled features
 	if src.Spec.Retention != nil && src.Spec.Retention.AtBuildDeletion != nil {
-		if alphaBuild.ObjectMeta.Annotations == nil {
-			alphaBuild.ObjectMeta.Annotations = make(map[string]string, 1)
+		// We must create a new Map as otherwise the addition is not kept
+		alphaBuild.ObjectMeta.Annotations = map[string]string{}
+		for k, v := range src.Annotations {
+			alphaBuild.ObjectMeta.Annotations[k] = v
 		}
 		alphaBuild.ObjectMeta.Annotations[v1alpha1.AnnotationBuildRunDeletion] = strconv.FormatBool(*src.Spec.Retention.AtBuildDeletion)
 	}
@@ -64,6 +68,9 @@ func (src *Build) ConvertFrom(ctx context.Context, obj *unstructured.Unstructure
 	if err != nil {
 		ctxlog.Error(ctx, err, "failed unstructuring the convertedObject")
 	}
+
+	ctxlog.Debug(ctx, "Converting Build from alpha to beta", "namespace", alphaBuild.Namespace, "name", alphaBuild.Name)
+
 	src.ObjectMeta = alphaBuild.ObjectMeta
 	src.TypeMeta = alphaBuild.TypeMeta
 	src.TypeMeta.APIVersion = betaGroupVersion
@@ -217,12 +224,14 @@ func (dest *BuildSpec) ConvertTo(bs *v1alpha1.BuildSpec) error {
 	// Handle BuildSpec ParamValues
 	bs.ParamValues = nil
 	for _, p := range dest.ParamValues {
+		if p.Name == "dockerfile" && p.SingleValue != nil {
+			bs.Dockerfile = p.SingleValue.Value
+			continue
+		}
+
 		param := v1alpha1.ParamValue{}
 		p.convertToAlpha(&param)
 		bs.ParamValues = append(bs.ParamValues, param)
-		if param.Name == "dockerfile" {
-			bs.Dockerfile = param.Value
-		}
 	}
 
 	// Handle BuildSpec Output
diff --git a/pkg/apis/build/v1beta1/buildrun_conversion.go b/pkg/apis/build/v1beta1/buildrun_conversion.go
index e91a5f4e..cb7d6481 100644
--- a/pkg/apis/build/v1beta1/buildrun_conversion.go
+++ b/pkg/apis/build/v1beta1/buildrun_conversion.go
@@ -20,6 +20,7 @@ var _ webhook.Conversion = (*BuildRun)(nil)
 
 // To Alpha
 func (src *BuildRun) ConvertTo(ctx context.Context, obj *unstructured.Unstructured) error {
+	ctxlog.Debug(ctx, "Converting BuildRun from beta to alpha", "namespace", src.Namespace, "name", src.Name)
 
 	var alphaBuildRun v1alpha1.BuildRun
 
@@ -110,6 +111,8 @@ func (src *BuildRun) ConvertFrom(ctx context.Context, obj *unstructured.Unstruct
 		ctxlog.Error(ctx, err, "failed unstructuring the buildrun convertedObject")
 	}
 
+	ctxlog.Debug(ctx, "Converting BuildRun from alpha to beta", "namespace", alphaBuildRun.Namespace, "name", alphaBuildRun.Name)
+
 	src.ObjectMeta = alphaBuildRun.ObjectMeta
 	src.TypeMeta = alphaBuildRun.TypeMeta
 	src.TypeMeta.APIVersion = betaGroupVersion
diff --git a/pkg/apis/build/v1beta1/buildstrategy_conversion.go b/pkg/apis/build/v1beta1/buildstrategy_conversion.go
index e0afd297..ae803999 100644
--- a/pkg/apis/build/v1beta1/buildstrategy_conversion.go
+++ b/pkg/apis/build/v1beta1/buildstrategy_conversion.go
@@ -6,6 +6,7 @@ package v1beta1
 
 import (
 	"context"
+	"strings"
 
 	"github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
 	"github.com/shipwright-io/build/pkg/ctxlog"
@@ -13,6 +14,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	runtime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/pointer"
 )
 
 // ensure v1beta1 implements the Conversion interface
@@ -20,6 +22,8 @@ var _ webhook.Conversion = (*BuildStrategy)(nil)
 
 // ConvertTo converts this object to its v1alpha1 equivalent
 func (src *BuildStrategy) ConvertTo(ctx context.Context, obj *unstructured.Unstructured) error {
+	ctxlog.Debug(ctx, "Converting BuildStrategy from beta to alpha", "namespace", src.Namespace, "name", src.Name)
+
 	var bs v1alpha1.BuildStrategy
 	bs.TypeMeta = src.TypeMeta
 	bs.TypeMeta.APIVersion = alphaGroupVersion
@@ -37,6 +41,23 @@ func (src *BuildStrategy) ConvertTo(ctx context.Context, obj *unstructured.Unstr
 }
 
 func (src *BuildStrategySpec) ConvertTo(bs *v1alpha1.BuildStrategySpec) {
+	usesMigratedDockerfileArg := false
+
+	bs.Parameters = []v1alpha1.Parameter{}
+	for _, param := range src.Parameters {
+		if param.Name == "dockerfile" && param.Type == ParameterTypeString && param.Default != nil && *param.Default == "Dockerfile" {
+			usesMigratedDockerfileArg = true
+			continue
+		}
+
+		bs.Parameters = append(bs.Parameters, v1alpha1.Parameter{
+			Name:        param.Name,
+			Description: param.Description,
+			Type:        v1alpha1.ParameterType(param.Type),
+			Default:     param.Default,
+			Defaults:    param.Defaults,
+		})
+	}
 
 	bs.BuildSteps = []v1alpha1.BuildStep{}
 	for _, step := range src.Steps {
@@ -52,27 +73,35 @@ func (src *BuildStrategySpec) ConvertTo(bs *v1alpha1.BuildStrategySpec) {
 				Resources:       step.Resources,
 				VolumeMounts:    step.VolumeMounts,
 				ImagePullPolicy: step.ImagePullPolicy,
+				SecurityContext: step.SecurityContext,
 			},
 		}
 
-		if step.SecurityContext != nil {
-			buildStep.SecurityContext = step.SecurityContext
+		if usesMigratedDockerfileArg {
+			// Migrate to legacy argument
+
+			for commandIndex, command := range buildStep.Command {
+				if strings.Contains(command, "$(params.dockerfile)") {
+					buildStep.Command[commandIndex] = strings.ReplaceAll(command, "$(params.dockerfile)", "$(params.DOCKERFILE)")
+				}
+			}
+
+			for argIndex, arg := range buildStep.Args {
+				if strings.Contains(arg, "$(params.dockerfile)") {
+					buildStep.Args[argIndex] = strings.ReplaceAll(arg, "$(params.dockerfile)", "$(params.DOCKERFILE)")
+				}
+			}
+
+			for envIndex, env := range buildStep.Env {
+				if strings.Contains(env.Value, "$(params.dockerfile)") {
+					buildStep.Env[envIndex].Value = strings.ReplaceAll(env.Value, "$(params.dockerfile)", "$(params.DOCKERFILE)")
+				}
+			}
 		}
 
 		bs.BuildSteps = append(bs.BuildSteps, buildStep)
 	}
 
-	bs.Parameters = []v1alpha1.Parameter{}
-	for _, param := range src.Parameters {
-		bs.Parameters = append(bs.Parameters, v1alpha1.Parameter{
-			Name:        param.Name,
-			Description: param.Description,
-			Type:        v1alpha1.ParameterType(param.Type),
-			Default:     param.Default,
-			Defaults:    param.Defaults,
-		})
-	}
-
 	if src.SecurityContext != nil {
 		bs.SecurityContext = (*v1alpha1.BuildStrategySecurityContext)(src.SecurityContext)
 	}
@@ -90,25 +119,30 @@ func (src *BuildStrategySpec) ConvertTo(bs *v1alpha1.BuildStrategySpec) {
 
 // ConvertFrom converts from v1alpha1.BuildStrategy into this object.
 func (src *BuildStrategy) ConvertFrom(ctx context.Context, obj *unstructured.Unstructured) error {
-	var br v1alpha1.BuildStrategy
+	var bs v1alpha1.BuildStrategy
 
 	unstructured := obj.UnstructuredContent()
-	err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructured, &br)
+	err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructured, &bs)
 	if err != nil {
 		ctxlog.Error(ctx, err, "failed unstructuring the buildrun convertedObject")
 	}
 
-	src.ObjectMeta = br.ObjectMeta
-	src.TypeMeta = br.TypeMeta
+	ctxlog.Debug(ctx, "Converting BuildStrategy from alpha to beta", "namespace", bs.Namespace, "name", bs.Name)
+
+	src.ObjectMeta = bs.ObjectMeta
+	src.TypeMeta = bs.TypeMeta
 	src.TypeMeta.APIVersion = betaGroupVersion
 
-	src.Spec.ConvertFrom(br.Spec)
+	src.Spec.ConvertFrom(bs.Spec)
 
 	return nil
 }
 
 func (src *BuildStrategySpec) ConvertFrom(bs v1alpha1.BuildStrategySpec) {
 	src.Steps = []Step{}
+
+	usesDockerfile := false
+
 	for _, brStep := range bs.BuildSteps {
 		step := Step{
 			Name:            brStep.Name,
@@ -122,6 +156,42 @@ func (src *BuildStrategySpec) ConvertFrom(bs v1alpha1.BuildStrategySpec) {
 			ImagePullPolicy: brStep.ImagePullPolicy,
 			SecurityContext: brStep.SecurityContext,
 		}
+
+		// Migrate legacy argument usage
+
+		for commandIndex, command := range step.Command {
+			if strings.Contains(command, "$(params.DOCKERFILE)") {
+				usesDockerfile = true
+				step.Command[commandIndex] = strings.ReplaceAll(command, "$(params.DOCKERFILE)", "$(params.dockerfile)")
+			}
+			if strings.Contains(command, "$(build.dockerfile)") {
+				usesDockerfile = true
+				step.Command[commandIndex] = strings.ReplaceAll(command, "$(build.dockerfile)", "$(params.dockerfile)")
+			}
+		}
+
+		for argIndex, arg := range step.Args {
+			if strings.Contains(arg, "$(params.DOCKERFILE)") {
+				usesDockerfile = true
+				step.Args[argIndex] = strings.ReplaceAll(arg, "$(params.DOCKERFILE)", "$(params.dockerfile)")
+			}
+			if strings.Contains(arg, "$(build.dockerfile)") {
+				usesDockerfile = true
+				step.Args[argIndex] = strings.ReplaceAll(arg, "$(build.dockerfile)", "$(params.dockerfile)")
+			}
+		}
+
+		for envIndex, env := range step.Env {
+			if strings.Contains(env.Value, "$(params.DOCKERFILE)") {
+				usesDockerfile = true
+				step.Env[envIndex].Value = strings.ReplaceAll(env.Value, "$(params.DOCKERFILE)", "$(params.dockerfile)")
+			}
+			if strings.Contains(env.Value, "$(build.dockerfile)") {
+				usesDockerfile = true
+				step.Env[envIndex].Value = strings.ReplaceAll(env.Value, "$(build.dockerfile)", "$(params.dockerfile)")
+			}
+		}
+
 		src.Steps = append(src.Steps, step)
 	}
 
@@ -136,6 +206,17 @@ func (src *BuildStrategySpec) ConvertFrom(bs v1alpha1.BuildStrategySpec) {
 		})
 	}
 
+	// Add replacement for legacy arguments
+
+	if usesDockerfile {
+		src.Parameters = append(src.Parameters, Parameter{
+			Name:        "dockerfile",
+			Description: "The Dockerfile to be built.",
+			Type:        ParameterTypeString,
+			Default:     pointer.String("Dockerfile"),
+		})
+	}
+
 	src.SecurityContext = (*BuildStrategySecurityContext)(bs.SecurityContext)
 
 	src.Volumes = []BuildStrategyVolume{}
diff --git a/pkg/apis/build/v1beta1/clusterbuildstrategy_conversion.go b/pkg/apis/build/v1beta1/clusterbuildstrategy_conversion.go
index 3b883b59..6a15007f 100644
--- a/pkg/apis/build/v1beta1/clusterbuildstrategy_conversion.go
+++ b/pkg/apis/build/v1beta1/clusterbuildstrategy_conversion.go
@@ -19,6 +19,8 @@ var _ webhook.Conversion = (*ClusterBuildStrategy)(nil)
 
 // ConvertTo converts this object to its v1alpha1 equivalent
 func (src *ClusterBuildStrategy) ConvertTo(ctx context.Context, obj *unstructured.Unstructured) error {
+	ctxlog.Debug(ctx, "Converting ClusterBuildStrategy from beta to alpha", "namespace", src.Namespace, "name", src.Name)
+
 	var bs v1alpha1.ClusterBuildStrategy
 	bs.TypeMeta = src.TypeMeta
 	bs.TypeMeta.APIVersion = alphaGroupVersion
@@ -37,19 +39,21 @@ func (src *ClusterBuildStrategy) ConvertTo(ctx context.Context, obj *unstructure
 
 // ConvertFrom converts v1alpha1.ClusterBuildStrategy into this object
 func (src *ClusterBuildStrategy) ConvertFrom(ctx context.Context, obj *unstructured.Unstructured) error {
-	var br v1alpha1.ClusterBuildStrategy
+	var cbs v1alpha1.ClusterBuildStrategy
 
 	unstructured := obj.UnstructuredContent()
-	err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructured, &br)
+	err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructured, &cbs)
 	if err != nil {
 		ctxlog.Error(ctx, err, "failed unstructuring the buildrun convertedObject")
 	}
 
-	src.ObjectMeta = br.ObjectMeta
-	src.TypeMeta = br.TypeMeta
+	ctxlog.Debug(ctx, "Converting ClusterBuildStrategy from alpha to beta", "namespace", cbs.Namespace, "name", cbs.Name)
+
+	src.ObjectMeta = cbs.ObjectMeta
+	src.TypeMeta = cbs.TypeMeta
 	src.TypeMeta.APIVersion = betaGroupVersion
 
-	src.Spec.ConvertFrom(br.Spec)
+	src.Spec.ConvertFrom(cbs.Spec)
 
 	return nil
 }
diff --git a/pkg/webhook/conversion/converter.go b/pkg/webhook/conversion/converter.go
index 2e502e31..c7aca331 100644
--- a/pkg/webhook/conversion/converter.go
+++ b/pkg/webhook/conversion/converter.go
@@ -80,7 +80,7 @@ func convertSHPCR(ctx context.Context, Object *unstructured.Unstructured, toVers
 				}
 				clusterBuildStrategy.ConvertTo(ctx, convertedObject)
 			} else {
-				return nil, statusErrorWithMessage("unsupporteddda skdksdjkjsd Kind")
+				return nil, statusErrorWithMessage("unsupported Kind")
 			}
 		default:
 			return nil, statusErrorWithMessage("unexpected conversion version to %q", toVersion)


Test patch:
diff --git a/pkg/webhook/conversion/converter_test.go b/pkg/webhook/conversion/converter_test.go
index 6c1e2502..f9d37803 100644
--- a/pkg/webhook/conversion/converter_test.go
+++ b/pkg/webhook/conversion/converter_test.go
@@ -259,12 +259,6 @@ request:
 						Duration: 10 * time.Minute,
 					},
 					ParamValues: []v1alpha1.ParamValue{
-						{
-							Name: "dockerfile",
-							SingleValue: &v1alpha1.SingleValue{
-								Value: &dockerfileVal,
-							},
-						},
 						{
 							Name: "foo1",
 							SingleValue: &v1alpha1.SingleValue{
@@ -978,6 +972,10 @@ request:
         steps:
         - name: step-foobar
           image: foobar
+          command:
+          - some-command
+          args:
+          - $(params.dockerfile)
           securityContext:
             privileged: false
         parameters:
@@ -987,6 +985,10 @@ request:
         - name: param_two
           description: foobar
           type: array
+        - name: dockerfile
+          description: The Dockerfile to build.
+          type: string
+          default: Dockerfile
         securityContext:
           runAsUser: 1000
           runAsGroup: 1000
@@ -1024,8 +1026,10 @@ request:
 					BuildSteps: []v1alpha1.BuildStep{
 						{
 							Container: corev1.Container{
-								Name:  "step-foobar",
-								Image: "foobar",
+								Name:    "step-foobar",
+								Command: []string{"some-command"},
+								Args:    []string{"$(params.DOCKERFILE)"},
+								Image:   "foobar",
 								SecurityContext: &corev1.SecurityContext{
 									Privileged: &privileged,
 								},
@@ -1079,6 +1083,10 @@ request:
       spec:
         buildSteps:
         - name: step-foobar
+          command:
+          - some-command
+          args:
+          - $(params.DOCKERFILE)
           image: foobar
           securityContext:
             privileged: false
@@ -1125,8 +1133,10 @@ request:
 				Spec: v1beta1.BuildStrategySpec{
 					Steps: []v1beta1.Step{
 						{
-							Name:  "step-foobar",
-							Image: "foobar",
+							Name:    "step-foobar",
+							Command: []string{"some-command"},
+							Args:    []string{"$(params.dockerfile)"},
+							Image:   "foobar",
 							SecurityContext: &corev1.SecurityContext{
 								Privileged: &privileged,
 							},
@@ -1143,6 +1153,12 @@ request:
 							Description: "foobar",
 							Type:        v1beta1.ParameterTypeArray,
 						},
+						{
+							Name:        "dockerfile",
+							Description: "The Dockerfile to be built.",
+							Type:        v1beta1.ParameterTypeString,
+							Default:     pointer.String("Dockerfile"),
+						},
 					},
 					SecurityContext: &v1beta1.BuildStrategySecurityContext{
 						RunAsUser:  1000,