CRD usage of pointers vs non-pointers

We were not following the pointer vs non-pointer fields for custom resource definitions having required and optional fields.
Fixes shipwright-io#397

Update required custom resource fields to non-pointer
Update optional custom resource fields that do not have a built-in nil value to pointers

deploy/crds/shipwright.io_buildruns.yaml

@@ -626,6 +626,8 @@ spec:
                 format: date-time
                 type: string
             type: object
+        required:
+        - spec
         type: object
     served: true
     storage: true

deploy/crds/shipwright.io_builds.yaml

@@ -339,6 +339,8 @@ spec:
                 description: The Register status of the Build
                 type: string
             type: object
+        required:
+        - spec
         type: object
     served: true
     storage: true

pkg/apis/build/v1alpha1/build_types.go

@@ -48,6 +48,16 @@ const (
 	AllValidationsSucceeded = "all validations succeeded"
 )
 
+// BuildReasonPtr returns a pointer to the passed BuildReason.
+func BuildReasonPtr(s BuildReason) *BuildReason {
+	return &s
+}
+
+// ConditionStatusPtr returns a pointer to the passed ConditionStatus.
+func ConditionStatusPtr(s corev1.ConditionStatus) *corev1.ConditionStatus {
+	return &s
+}
+
 const (
 	// BuildDomain is the domain used for all labels and annotations for this resource
 	BuildDomain = "build.shipwright.io"
@@ -81,11 +91,11 @@ type BuildSpec struct {
 	// (`.spec.source`) data.
 	//
 	// +optional
-	Sources *[]BuildSource `json:"sources,omitempty"`
+	Sources []BuildSource `json:"sources,omitempty"`
 
 	// Strategy references the BuildStrategy to use to build the container
 	// image.
-	Strategy *Strategy `json:"strategy"`
+	Strategy Strategy `json:"strategy"`
 
 	// Builder refers to the image containing the build tools inside which
 	// the source code would be built.
@@ -126,10 +136,6 @@ func (buildSpec *BuildSpec) StrategyName() string {
 		return "undefined (nil buildSpec)"
 	}
 
-	if buildSpec.Strategy == nil {
-		return "undefined (nil strategy)"
-	}
-
 	return buildSpec.Strategy.Name
 }
 
@@ -159,15 +165,15 @@ type Image struct {
 type BuildStatus struct {
 	// The Register status of the Build
 	// +optional
-	Registered corev1.ConditionStatus `json:"registered,omitempty"`
+	Registered *corev1.ConditionStatus `json:"registered,omitempty"`
 
 	// The reason of the registered Build, it's an one-word camelcase
 	// +optional
-	Reason BuildReason `json:"reason,omitempty"`
+	Reason *BuildReason `json:"reason,omitempty"`
 
 	// The message of the registered Build, either an error or succeed message
 	// +optional
-	Message string `json:"message,omitempty"`
+	Message *string `json:"message,omitempty"`
 }
 
 // +genclient
@@ -185,7 +191,7 @@ type Build struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 
-	Spec   BuildSpec   `json:"spec,omitempty"`
+	Spec   BuildSpec   `json:"spec"`
 	Status BuildStatus `json:"status,omitempty"`
 }
 

pkg/apis/build/v1alpha1/buildrun_types.go

@@ -23,7 +23,7 @@ const (
 // BuildRunSpec defines the desired state of BuildRun
 type BuildRunSpec struct {
 	// BuildRef refers to the Build
-	BuildRef *BuildRef `json:"buildRef"`
+	BuildRef BuildRef `json:"buildRef"`
 
 	// ServiceAccount refers to the kubernetes serviceaccount
 	// which is used for resource control.
@@ -48,7 +48,7 @@ type BuildRunSpec struct {
 
 	// State is used for canceling a buildrun (and maybe more later on).
 	// +optional
-	State BuildRunRequestedState `json:"state,omitempty"`
+	State *BuildRunRequestedState `json:"state,omitempty"`
 
 	// Env contains additional environment variables that should be passed to the build container
 	// +optional
@@ -58,6 +58,11 @@ type BuildRunSpec struct {
 // BuildRunRequestedState defines the buildrun state the user can provide to override whatever is the current state.
 type BuildRunRequestedState string
 
+// BuildRunRequestedStatePtr returns a pointer to the passed BuildRunRequestedState.
+func BuildRunRequestedStatePtr(s BuildRunRequestedState) *BuildRunRequestedState {
+	return &s
+}
+
 const (
 	// BuildRunStateCancel indicates that the user wants to cancel the BuildRun,
 	// if not already canceled or terminated
@@ -166,7 +171,7 @@ type BuildRef struct {
 	Name string `json:"name"`
 	// API version of the referent
 	// +optional
-	APIVersion string `json:"apiVersion,omitempty"`
+	APIVersion *string `json:"apiVersion,omitempty"`
 }
 
 // ServiceAccount can be used to refer to a specific ServiceAccount.
@@ -176,7 +181,7 @@ type ServiceAccount struct {
 	Name *string `json:"name,omitempty"`
 	// If generates a new ServiceAccount for the build
 	// +optional
-	Generate bool `json:"generate,omitempty"`
+	Generate *bool `json:"generate,omitempty"`
 }
 
 // +genclient
@@ -193,7 +198,7 @@ type BuildRun struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 
-	Spec   BuildRunSpec   `json:"spec,omitempty"`
+	Spec   BuildRunSpec   `json:"spec"`
 	Status BuildRunStatus `json:"status,omitempty"`
 }
 
@@ -225,7 +230,7 @@ func (br *BuildRun) IsSuccessful() bool {
 
 // IsCanceled returns true if the BuildRun's spec status is set to BuildRunCanceled state.
 func (br *BuildRun) IsCanceled() bool {
-	return br.Spec.State == BuildRunStateCancel
+	return br.Spec.State != nil && *br.Spec.State == BuildRunStateCancel
 }
 
 // Conditions defines a list of Condition
@@ -253,15 +258,15 @@ type Condition struct {
 
 	// LastTransitionTime last time the condition transit from one status to another.
 	// +optional
-	LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"`
+	LastTransitionTime *metav1.Time `json:"lastTransitionTime,omitempty" description:"last time the condition transit from one status to another"`
 
 	// The reason for the condition last transition.
 	// +optional
-	Reason string `json:"reason,omitempty" description:"one-word CamelCase reason for the condition's last transition"`
+	Reason *string `json:"reason,omitempty" description:"one-word CamelCase reason for the condition's last transition"`
 
 	// A human readable message indicating details about the transition.
 	// +optional
-	Message string `json:"message,omitempty" description:"human-readable message indicating details about last transition"`
+	Message *string `json:"message,omitempty" description:"human-readable message indicating details about last transition"`
 }
 
 func init() {
@@ -274,7 +279,7 @@ func (c *Condition) GetReason() string {
 	if c == nil {
 		return ""
 	}
-	return c.Reason
+	return *c.Reason
 }
 
 // GetMessage returns the condition Message, it ensures that by getting the Message
@@ -283,7 +288,7 @@ func (c *Condition) GetMessage() string {
 	if c == nil {
 		return ""
 	}
-	return c.Message
+	return *c.Message
 }
 
 // GetStatus returns the condition Status, it ensures that by getting the Status

pkg/apis/build/v1alpha1/buildstrategy.go

@@ -38,7 +38,7 @@ type Parameter struct {
 
 	// Reasonable default value for the parameter
 	// +optional
-	Default *string `json:"default"`
+	Default *string `json:"default,omitempty"`
 }
 
 // BuildStep defines a partial step that needs to run in container for building the image.
@@ -68,7 +68,7 @@ type Strategy struct {
 
 	// API version of the referent
 	// +optional
-	APIVersion string `json:"apiVersion,omitempty"`
+	APIVersion *string `json:"apiVersion,omitempty"`
 }
 
 // BuilderStrategy defines the common elements of build strategies

pkg/apis/build/v1alpha1/source.go

@@ -19,7 +19,7 @@ type Source struct {
 	// URL describes the URL of the Git repository.
 	//
 	// +optional
-	URL string `json:"url,omitempty"`
+	URL *string `json:"url,omitempty"`
 
 	// BundleContainer
 	//

pkg/reconciler/build/build.go

@@ -10,6 +10,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -60,8 +61,8 @@ func (r *ReconcileBuild) Reconcile(ctx context.Context, request reconcile.Reques
 	}
 
 	// Populate the status struct with default values
-	b.Status.Registered = corev1.ConditionFalse
-	b.Status.Reason = build.SucceedStatus
+	b.Status.Registered = build.ConditionStatusPtr(corev1.ConditionFalse)
+	b.Status.Reason = build.BuildReasonPtr(build.SucceedStatus)
 
 	// build a list of current validation types
 	validationTypes := []string{
@@ -95,13 +96,13 @@ func (r *ReconcileBuild) Reconcile(ctx context.Context, request reconcile.Reques
 				ctxlog.Info(ctx, "unexpected error during ownership reference validation", namespace, request.Namespace, name, request.Name, "error", err)
 			}
 		}
-		if b.Status.Reason != build.SucceedStatus {
+		if b.Status.Reason == nil || *b.Status.Reason != build.SucceedStatus {
 			return r.UpdateBuildStatusAndRetreat(ctx, b)
 		}
 	}
 
-	b.Status.Registered = corev1.ConditionTrue
-	b.Status.Message = build.AllValidationsSucceeded
+	b.Status.Registered = build.ConditionStatusPtr(corev1.ConditionTrue)
+	b.Status.Message = pointer.StringPtr(build.AllValidationsSucceeded)
 	err = r.client.Status().Update(ctx, b)
 	if err != nil {
 		return reconcile.Result{}, err

pkg/reconciler/buildrun/buildrun.go

@@ -21,6 +21,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -132,16 +133,22 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req
 			}
 
 			// Validate if the Build was successfully registered
-			if build.Status.Registered == "" {
+			if build.Status.Registered == nil || *build.Status.Registered == "" {
 				err := fmt.Errorf("the Build is not yet validated, build: %s", build.Name)
 				// reconcile again until it gets a registration value
 				return reconcile.Result{}, err
 			}
 
-			if build.Status.Registered != corev1.ConditionTrue {
+			if build.Status.Registered != nil && *build.Status.Registered != corev1.ConditionTrue {
 				// stop reconciling and mark the BuildRun as Failed
 				// we only reconcile again if the status.Update call fails
-				message := fmt.Sprintf("the Build is not registered correctly, build: %s, registered status: %s, reason: %s", build.Name, build.Status.Registered, build.Status.Reason)
+				var reason buildv1alpha1.BuildReason
+
+				if build.Status.Reason != nil {
+					reason = *build.Status.Reason
+				}
+
+				message := fmt.Sprintf("the Build is not registered correctly, build: %s, registered status: %s, reason: %s", build.Name, *build.Status.Registered, reason)
 				if updateErr := resources.UpdateConditionWithFalseStatus(ctx, r.client, buildRun, message, resources.ConditionBuildRegistrationFailed); updateErr != nil {
 					return reconcile.Result{}, updateErr
 				}
@@ -165,8 +172,8 @@ func (r *ReconcileBuildRun) Reconcile(ctx context.Context, request reconcile.Req
 			// Set OwnerReference for Build and BuildRun only when build.shipwright.io/build-run-deletion is set "true"
 			if build.GetAnnotations()[buildv1alpha1.AnnotationBuildRunDeletion] == "true" && !resources.IsOwnedByBuild(build, buildRun.OwnerReferences) {
 				if err := r.setOwnerReferenceFunc(build, buildRun, r.scheme); err != nil {
-					build.Status.Reason = buildv1alpha1.SetOwnerReferenceFailed
-					build.Status.Message = fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err)
+					build.Status.Reason = buildv1alpha1.BuildReasonPtr(buildv1alpha1.SetOwnerReferenceFailed)
+					build.Status.Message = pointer.StringPtr(fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err))
 					if err := r.client.Status().Update(ctx, build); err != nil {
 						return reconcile.Result{}, err
 					}

pkg/reconciler/buildrun/resources/conditions.go

@@ -7,6 +7,7 @@ package resources
 import (
 	"context"
 	"fmt"
+
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"knative.dev/pkg/apis"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -124,12 +125,14 @@ func UpdateBuildRunUsingTaskRunCondition(ctx context.Context, client client.Clie
 		}
 	}
 
+	lastTransitionTime := metav1.Now()
+
 	buildRun.Status.SetCondition(&buildv1alpha1.Condition{
-		LastTransitionTime: metav1.Now(),
+		LastTransitionTime: &lastTransitionTime,
 		Type:               buildv1alpha1.Succeeded,
 		Status:             status,
-		Reason:             reason,
-		Message:            message,
+		Reason:             &reason,
+		Message:            &message,
 	})
 
 	return nil
@@ -142,11 +145,11 @@ func UpdateConditionWithFalseStatus(ctx context.Context, client client.Client, b
 	now := metav1.Now()
 	buildRun.Status.CompletionTime = &now
 	buildRun.Status.SetCondition(&buildv1alpha1.Condition{
-		LastTransitionTime: now,
+		LastTransitionTime: &now,
 		Type:               buildv1alpha1.Succeeded,
 		Status:             corev1.ConditionFalse,
-		Reason:             reason,
-		Message:            errorMessage,
+		Reason:             &reason,
+		Message:            &errorMessage,
 	})
 	ctxlog.Debug(ctx, "updating buildRun status", namespace, buildRun.Namespace, name, buildRun.Name, "reason", reason)
 	if err := client.Status().Update(ctx, buildRun); err != nil {

pkg/reconciler/buildrun/resources/service_accounts.go

@@ -36,7 +36,7 @@ func GetGeneratedServiceAccountName(buildRun *buildv1alpha1.BuildRun) string {
 
 // IsGeneratedServiceAccountUsed checks if a build run uses a generated service account
 func IsGeneratedServiceAccountUsed(buildRun *buildv1alpha1.BuildRun) bool {
-	return buildRun.Spec.ServiceAccount != nil && buildRun.Spec.ServiceAccount.Generate
+	return buildRun.Spec.ServiceAccount != nil && buildRun.Spec.ServiceAccount.Generate != nil && *buildRun.Spec.ServiceAccount.Generate
 }
 
 // GenerateSA generates a new service account on the fly

pkg/reconciler/buildrun/resources/sources.go

@@ -25,16 +25,14 @@ func AmendTaskSpecWithSources(
 	case build.Spec.Source.BundleContainer != nil:
 		sources.AppendBundleStep(cfg, taskSpec, build.Spec.Source, defaultSourceName)
 
-	case build.Spec.Source.URL != "":
+	case build.Spec.Source.URL != nil:
 		sources.AppendGitStep(cfg, taskSpec, build.Spec.Source, defaultSourceName)
 	}
 
 	// create the step for spec.sources, this will eventually change into different steps depending on the type of the source
-	if build.Spec.Sources != nil {
-		for _, source := range *build.Spec.Sources {
-			// today, we only have HTTP sources
-			sources.AppendHTTPStep(cfg, taskSpec, source)
-		}
+	for _, source := range build.Spec.Sources {
+		// today, we only have HTTP sources
+		sources.AppendHTTPStep(cfg, taskSpec, source)
 	}
 }
 
@@ -45,7 +43,7 @@ func updateBuildRunStatusWithSourceResult(buildrun *buildv1alpha1.BuildRun, resu
 	case buildSpec.Source.BundleContainer != nil:
 		sources.AppendBundleResult(buildrun, defaultSourceName, results)
 
-	case buildSpec.Source.URL != "":
+	case buildSpec.Source.URL != nil:
 		sources.AppendGitResult(buildrun, defaultSourceName, results)
 	}
 

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

@@ -48,7 +48,7 @@ func AppendGitStep(
 	gitStep.Container.Name = fmt.Sprintf("source-%s", name)
 	gitStep.Container.Args = []string{
 		"--url",
-		source.URL,
+		*source.URL,
 		"--target",
 		fmt.Sprintf("$(params.%s-%s)", prefixParamsResultsVolumes, paramSourceRoot),
 		"--result-file-commit-sha",