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

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

deploy/crds/shipwright.io_builds.yaml

@@ -431,6 +431,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

@@ -58,6 +58,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"
@@ -91,11 +101,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.
@@ -137,10 +147,6 @@ func (buildSpec *BuildSpec) StrategyName() string {
 		return "undefined (nil buildSpec)"
 	}
 
-	if buildSpec.Strategy == nil {
-		return "undefined (nil strategy)"
-	}
-
 	return buildSpec.Strategy.Name
 }
 
@@ -170,15 +176,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
@@ -196,7 +202,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"`
 
 	// Sources slice of BuildSource, defining external build artifacts complementary to VCS
 	// (`.spec.source`) data.
@@ -54,7 +54,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
@@ -64,6 +64,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
@@ -183,7 +188,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.
@@ -193,7 +198,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
@@ -210,7 +215,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"`
 }
 
@@ -242,7 +247,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
@@ -270,15 +275,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() {
@@ -291,7 +296,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
@@ -300,7 +305,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

@@ -52,7 +52,7 @@ type Parameter struct {
 
 	// Default value for a string parameter
 	// +optional
-	Default *string `json:"default"`
+	Default *string `json:"default,omitempty"`
 
 	// Default values for an array parameter
 	// +optional
@@ -63,7 +63,7 @@ type Parameter struct {
 // If the build step declares a volumeMount, Shipwright will create an emptyDir volume mount for the named volume.
 // Build steps which share the same named volume in the volumeMount will share the same underlying emptyDir volume.
 // This behavior is deprecated, and will be removed when full volume support is added to build strategies as specified
-// in SHIP-0022. 
+// in SHIP-0022.
 type BuildStep struct {
 	corev1.Container `json:",inline"`
 }
@@ -86,7 +86,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

@@ -117,12 +117,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
@@ -135,11 +137,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

@@ -22,7 +22,7 @@ func isLocalCopyBuildSource(
 ) *buildv1alpha1.BuildSource {
 	sources := []buildv1alpha1.BuildSource{}
 	if build.Spec.Sources != nil {
-		sources = append(sources, *build.Spec.Sources...)
+		sources = append(sources, build.Spec.Sources...)
 	}
 	if buildRun.Spec.Sources != nil {
 		sources = append(sources, *buildRun.Spec.Sources...)
@@ -50,18 +50,16 @@ func AmendTaskSpecWithSources(
 		switch {
 		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)
 		}
 	}
 
-	if build.Spec.Sources != nil {
-		// inspecting .spec.sources looking for "http" typed sources to generate the TaskSpec items
-		// in order to handle remote artifacts
-		for _, source := range *build.Spec.Sources {
-			if source.Type == buildv1alpha1.HTTP {
-				sources.AppendHTTPStep(cfg, taskSpec, source)
-			}
+	// inspecting .spec.sources looking for "http" typed sources to generate the TaskSpec items
+	// in order to handle remote artifacts
+	for _, source := range build.Spec.Sources {
+		if source.Type == buildv1alpha1.HTTP {
+			sources.AppendHTTPStep(cfg, taskSpec, source)
 		}
 	}
 }
@@ -73,7 +71,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)
 	}