From c6389c1542451c43b4c72949ae6c4da1152dff79 Mon Sep 17 00:00:00 2001 From: Shahul Date: Thu, 18 Nov 2021 10:39:01 +0530 Subject: [PATCH] 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 https://github.com/shipwright-io/build/issues/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 | 2 + deploy/crds/shipwright.io_builds.yaml | 2 + pkg/apis/build/v1alpha1/build_types.go | 26 +++++--- pkg/apis/build/v1alpha1/buildrun_types.go | 27 ++++---- pkg/apis/build/v1alpha1/buildstrategy.go | 4 +- pkg/apis/build/v1alpha1/source.go | 2 +- pkg/reconciler/build/build.go | 11 ++-- pkg/reconciler/buildrun/buildrun.go | 17 +++-- .../buildrun/resources/conditions.go | 15 +++-- .../buildrun/resources/service_accounts.go | 2 +- pkg/reconciler/buildrun/resources/sources.go | 12 ++-- .../buildrun/resources/sources/git.go | 2 +- pkg/validate/buildname.go | 5 +- pkg/validate/envvars.go | 13 ++-- pkg/validate/ownerreferences.go | 5 +- pkg/validate/secrets.go | 9 +-- pkg/validate/sources.go | 7 +-- pkg/validate/sourceurl.go | 9 +-- pkg/validate/strategies.go | 63 +++++++++---------- 19 files changed, 128 insertions(+), 105 deletions(-) diff --git a/deploy/crds/shipwright.io_buildruns.yaml b/deploy/crds/shipwright.io_buildruns.yaml index 90dcac7713..d66bcb4fd9 100644 --- a/deploy/crds/shipwright.io_buildruns.yaml +++ b/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 diff --git a/deploy/crds/shipwright.io_builds.yaml b/deploy/crds/shipwright.io_builds.yaml index d0a77e784e..27456decd5 100644 --- a/deploy/crds/shipwright.io_builds.yaml +++ b/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 diff --git a/pkg/apis/build/v1alpha1/build_types.go b/pkg/apis/build/v1alpha1/build_types.go index da1d6d871e..5fca2a8a93 100644 --- a/pkg/apis/build/v1alpha1/build_types.go +++ b/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"` } diff --git a/pkg/apis/build/v1alpha1/buildrun_types.go b/pkg/apis/build/v1alpha1/buildrun_types.go index 2e144c1d90..b986eeea79 100644 --- a/pkg/apis/build/v1alpha1/buildrun_types.go +++ b/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 diff --git a/pkg/apis/build/v1alpha1/buildstrategy.go b/pkg/apis/build/v1alpha1/buildstrategy.go index b824a1fbd3..0e98bb1d65 100644 --- a/pkg/apis/build/v1alpha1/buildstrategy.go +++ b/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 diff --git a/pkg/apis/build/v1alpha1/source.go b/pkg/apis/build/v1alpha1/source.go index af2ac75580..b80e24f429 100644 --- a/pkg/apis/build/v1alpha1/source.go +++ b/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 // diff --git a/pkg/reconciler/build/build.go b/pkg/reconciler/build/build.go index 7a4ef41717..b42569a7f7 100644 --- a/pkg/reconciler/build/build.go +++ b/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 diff --git a/pkg/reconciler/buildrun/buildrun.go b/pkg/reconciler/buildrun/buildrun.go index 71454e6c3b..453ae58313 100644 --- a/pkg/reconciler/buildrun/buildrun.go +++ b/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 } diff --git a/pkg/reconciler/buildrun/resources/conditions.go b/pkg/reconciler/buildrun/resources/conditions.go index d20cd9e1c6..af9b33db76 100644 --- a/pkg/reconciler/buildrun/resources/conditions.go +++ b/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 { diff --git a/pkg/reconciler/buildrun/resources/service_accounts.go b/pkg/reconciler/buildrun/resources/service_accounts.go index 504dcea5db..0d377c4a93 100644 --- a/pkg/reconciler/buildrun/resources/service_accounts.go +++ b/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 diff --git a/pkg/reconciler/buildrun/resources/sources.go b/pkg/reconciler/buildrun/resources/sources.go index 22fd44745e..7629c72676 100644 --- a/pkg/reconciler/buildrun/resources/sources.go +++ b/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) } diff --git a/pkg/reconciler/buildrun/resources/sources/git.go b/pkg/reconciler/buildrun/resources/sources/git.go index 7469851679..dd1fa5e813 100644 --- a/pkg/reconciler/buildrun/resources/sources/git.go +++ b/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", diff --git a/pkg/validate/buildname.go b/pkg/validate/buildname.go index 3c68f6da94..ee5eeff2d4 100644 --- a/pkg/validate/buildname.go +++ b/pkg/validate/buildname.go @@ -11,6 +11,7 @@ import ( build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/pointer" ) // BuildNameRef contains all required fields @@ -23,8 +24,8 @@ type BuildNameRef struct { // that build name is a valid label value func (b *BuildNameRef) ValidatePath(_ context.Context) error { if errs := validation.IsValidLabelValue(b.Build.Name); len(errs) > 0 { - b.Build.Status.Reason = build.BuildNameInvalid - b.Build.Status.Message = strings.Join(errs, ", ") + b.Build.Status.Reason = build.BuildReasonPtr(build.BuildNameInvalid) + b.Build.Status.Message = pointer.StringPtr(strings.Join(errs, ", ")) } return nil diff --git a/pkg/validate/envvars.go b/pkg/validate/envvars.go index 4bc1d58c81..46e742caba 100644 --- a/pkg/validate/envvars.go +++ b/pkg/validate/envvars.go @@ -10,6 +10,7 @@ import ( corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/utils/pointer" build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" ) @@ -45,15 +46,15 @@ func (e *Env) validate(envVar corev1.EnvVar) []error { var allErrs []error if envVar.Name == "" { - e.Build.Status.Reason = build.SpecEnvNameCanNotBeBlank - e.Build.Status.Message = "name for environment variable must not be blank" - allErrs = append(allErrs, fmt.Errorf("%s", e.Build.Status.Message)) + e.Build.Status.Reason = build.BuildReasonPtr(build.SpecEnvNameCanNotBeBlank) + e.Build.Status.Message = pointer.StringPtr("name for environment variable must not be blank") + allErrs = append(allErrs, fmt.Errorf("%s", *e.Build.Status.Message)) } if envVar.Value != "" && envVar.ValueFrom != nil { - e.Build.Status.Reason = build.SpecEnvOnlyOneOfValueOrValueFromMustBeSpecified - e.Build.Status.Message = "only one of value or valueFrom must be specified" - allErrs = append(allErrs, fmt.Errorf("%s", e.Build.Status.Message)) + e.Build.Status.Reason = build.BuildReasonPtr(build.SpecEnvOnlyOneOfValueOrValueFromMustBeSpecified) + e.Build.Status.Message = pointer.StringPtr("only one of value or valueFrom must be specified") + allErrs = append(allErrs, fmt.Errorf("%s", *e.Build.Status.Message)) } return allErrs diff --git a/pkg/validate/ownerreferences.go b/pkg/validate/ownerreferences.go index 4d4317902d..a34b0421f5 100644 --- a/pkg/validate/ownerreferences.go +++ b/pkg/validate/ownerreferences.go @@ -13,6 +13,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) @@ -40,8 +41,8 @@ func (o OwnerRef) ValidatePath(ctx context.Context) error { for _, buildRun := range buildRunList.Items { if index := o.validateBuildOwnerReference(buildRun.OwnerReferences); index == -1 { if err := controllerutil.SetControllerReference(o.Build, &buildRun, o.Scheme); err != nil { - o.Build.Status.Reason = build.SetOwnerReferenceFailed - o.Build.Status.Message = fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err) + o.Build.Status.Reason = build.BuildReasonPtr(build.SetOwnerReferenceFailed) + o.Build.Status.Message = pointer.StringPtr(fmt.Sprintf("unexpected error when trying to set the ownerreference: %v", err)) } if err = o.Client.Update(ctx, &buildRun); err != nil { return err diff --git a/pkg/validate/secrets.go b/pkg/validate/secrets.go index c16c3e27ba..13e53945f9 100644 --- a/pkg/validate/secrets.go +++ b/pkg/validate/secrets.go @@ -14,6 +14,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -36,8 +37,8 @@ func (s Credentials) ValidatePath(ctx context.Context) error { if err := s.Client.Get(ctx, types.NamespacedName{Name: refSecret, Namespace: s.Build.Namespace}, secret); err != nil && !apierrors.IsNotFound(err) { return err } else if apierrors.IsNotFound(err) { - s.Build.Status.Reason = secretType - s.Build.Status.Message = fmt.Sprintf("referenced secret %s not found", refSecret) + s.Build.Status.Reason = build.BuildReasonPtr(secretType) + s.Build.Status.Message = pointer.StringPtr(fmt.Sprintf("referenced secret %s not found", refSecret)) missingSecrets = append(missingSecrets, refSecret) } } @@ -46,8 +47,8 @@ func (s Credentials) ValidatePath(ctx context.Context) error { sort.Strings(missingSecrets) if len(missingSecrets) > 1 { - s.Build.Status.Reason = build.MultipleSecretRefNotFound - s.Build.Status.Message = fmt.Sprintf("missing secrets are %s", strings.Join(missingSecrets, ",")) + s.Build.Status.Reason = build.BuildReasonPtr(build.MultipleSecretRefNotFound) + s.Build.Status.Message = pointer.StringPtr(fmt.Sprintf("missing secrets are %s", strings.Join(missingSecrets, ","))) } return nil } diff --git a/pkg/validate/sources.go b/pkg/validate/sources.go index 2067d25d99..8e8d37055e 100644 --- a/pkg/validate/sources.go +++ b/pkg/validate/sources.go @@ -20,12 +20,7 @@ type SourcesRef struct { // ValidatePath executes the validation routine, inspecting the `build.spec.sources` path, which // contains a slice of BuildSource. func (s *SourcesRef) ValidatePath(_ context.Context) error { - if s.Build.Spec.Sources == nil { - return nil - } - sources := *s.Build.Spec.Sources - - for _, source := range sources { + for _, source := range s.Build.Spec.Sources { if err := s.validateSourceEntry(source); err != nil { return err } diff --git a/pkg/validate/sourceurl.go b/pkg/validate/sourceurl.go index 7cf100258c..a9a5141959 100644 --- a/pkg/validate/sourceurl.go +++ b/pkg/validate/sourceurl.go @@ -11,6 +11,7 @@ import ( build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" "github.com/shipwright-io/build/pkg/ctxlog" "github.com/shipwright-io/build/pkg/git" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -28,7 +29,7 @@ func (s SourceURLRef) ValidatePath(ctx context.Context) error { if s.Build.Spec.Source.Credentials == nil { switch s.Build.GetAnnotations()[build.AnnotationBuildVerifyRepository] { case "true": - if err := git.ValidateGitURLExists(ctx, s.Build.Spec.Source.URL); err != nil { + if err := git.ValidateGitURLExists(ctx, *s.Build.Spec.Source.URL); err != nil { s.MarkBuildStatus(s.Build, build.RemoteRepositoryUnreachable, err.Error()) return err } @@ -48,7 +49,7 @@ func (s SourceURLRef) ValidatePath(ctx context.Context) error { } // MarkBuildStatus updates a Build Status fields -func (s SourceURLRef) MarkBuildStatus(build *build.Build, reason build.BuildReason, msg string) { - build.Status.Reason = reason - build.Status.Message = msg +func (s SourceURLRef) MarkBuildStatus(b *build.Build, reason build.BuildReason, msg string) { + b.Status.Reason = build.BuildReasonPtr(reason) + b.Status.Message = pointer.StringPtr(msg) } diff --git a/pkg/validate/strategies.go b/pkg/validate/strategies.go index e5cf2171a4..cf44b721d8 100644 --- a/pkg/validate/strategies.go +++ b/pkg/validate/strategies.go @@ -14,6 +14,7 @@ import ( "github.com/shipwright-io/build/pkg/reconciler/buildrun/resources" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -28,36 +29,34 @@ type Strategy struct { // that the referenced strategy exists. This applies to both // namespaced or cluster scoped strategies func (s Strategy) ValidatePath(ctx context.Context) error { - if s.Build.Spec.Strategy != nil { - buildStrategy := &build.BuildStrategy{} - if s.Build.Spec.Strategy.Kind != nil { - switch *s.Build.Spec.Strategy.Kind { - case build.NamespacedBuildStrategyKind: - if err := s.validateBuildStrategy(ctx, s.Build.Spec.Strategy.Name, buildStrategy); err != nil { - return err - } - if err := s.validateBuildParams(buildStrategy.Spec.Parameters); err != nil { - return err - } - case build.ClusterBuildStrategyKind: - clusterBuildStrategy := &build.ClusterBuildStrategy{} - if err := s.validateClusterBuildStrategy(ctx, s.Build.Spec.Strategy.Name, clusterBuildStrategy); err != nil { - return err - } - if err := s.validateBuildParams(clusterBuildStrategy.Spec.Parameters); err != nil { - return err - } - default: - return fmt.Errorf("unknown strategy kind: %v", *s.Build.Spec.Strategy.Kind) - } - } else { - ctxlog.Info(ctx, "buildStrategy kind is nil, use default NamespacedBuildStrategyKind", namespace, s.Build.Namespace, name, s.Build.Name) + buildStrategy := &build.BuildStrategy{} + if s.Build.Spec.Strategy.Kind != nil { + switch *s.Build.Spec.Strategy.Kind { + case build.NamespacedBuildStrategyKind: if err := s.validateBuildStrategy(ctx, s.Build.Spec.Strategy.Name, buildStrategy); err != nil { return err } if err := s.validateBuildParams(buildStrategy.Spec.Parameters); err != nil { return err } + case build.ClusterBuildStrategyKind: + clusterBuildStrategy := &build.ClusterBuildStrategy{} + if err := s.validateClusterBuildStrategy(ctx, s.Build.Spec.Strategy.Name, clusterBuildStrategy); err != nil { + return err + } + if err := s.validateBuildParams(clusterBuildStrategy.Spec.Parameters); err != nil { + return err + } + default: + return fmt.Errorf("unknown strategy kind: %v", *s.Build.Spec.Strategy.Kind) + } + } else { + ctxlog.Info(ctx, "buildStrategy kind is nil, use default NamespacedBuildStrategyKind", namespace, s.Build.Namespace, name, s.Build.Name) + if err := s.validateBuildStrategy(ctx, s.Build.Spec.Strategy.Name, buildStrategy); err != nil { + return err + } + if err := s.validateBuildParams(buildStrategy.Spec.Parameters); err != nil { + return err } } return nil @@ -67,8 +66,8 @@ func (s Strategy) validateBuildStrategy(ctx context.Context, strategyName string if err := s.Client.Get(ctx, types.NamespacedName{Name: strategyName, Namespace: s.Build.Namespace}, buildStrategy); err != nil && !apierrors.IsNotFound(err) { return err } else if apierrors.IsNotFound(err) { - s.Build.Status.Reason = build.BuildStrategyNotFound - s.Build.Status.Message = fmt.Sprintf("buildStrategy %s does not exist in namespace %s", s.Build.Spec.Strategy.Name, s.Build.Namespace) + s.Build.Status.Reason = build.BuildReasonPtr(build.BuildStrategyNotFound) + s.Build.Status.Message = pointer.StringPtr(fmt.Sprintf("buildStrategy %s does not exist in namespace %s", s.Build.Spec.Strategy.Name, s.Build.Namespace)) } return nil @@ -78,8 +77,8 @@ func (s Strategy) validateClusterBuildStrategy(ctx context.Context, strategyName if err := s.Client.Get(ctx, types.NamespacedName{Name: strategyName}, clusterBuildStrategy); err != nil && !apierrors.IsNotFound(err) { return err } else if apierrors.IsNotFound(err) { - s.Build.Status.Reason = build.ClusterBuildStrategyNotFound - s.Build.Status.Message = fmt.Sprintf("clusterBuildStrategy %s does not exist", s.Build.Spec.Strategy.Name) + s.Build.Status.Reason = build.BuildReasonPtr(build.ClusterBuildStrategyNotFound) + s.Build.Status.Message = pointer.StringPtr(fmt.Sprintf("clusterBuildStrategy %s does not exist", s.Build.Spec.Strategy.Name)) } return nil } @@ -110,8 +109,8 @@ func (s Strategy) validateParamsNamesDefinition() { } if len(undesiredParams) > 0 { - s.Build.Status.Reason = build.RestrictedParametersInUse - s.Build.Status.Message = fmt.Sprintf("restricted parameters in use: %s", strings.Join(undesiredParams, ",")) + s.Build.Status.Reason = build.BuildReasonPtr(build.RestrictedParametersInUse) + s.Build.Status.Message = pointer.StringPtr(fmt.Sprintf("restricted parameters in use: %s", strings.Join(undesiredParams, ","))) } } @@ -132,7 +131,7 @@ func (s Strategy) validateParamsInStrategies(params []build.ParamValue, paramete } if len(undefinedParams) > 0 { - s.Build.Status.Reason = build.UndefinedParameter - s.Build.Status.Message = fmt.Sprintf("parameter not defined in the strategies: %s", strings.Join(undefinedParams, ",")) + s.Build.Status.Reason = build.BuildReasonPtr(build.UndefinedParameter) + s.Build.Status.Message = pointer.StringPtr(fmt.Sprintf("parameter not defined in the strategies: %s", strings.Join(undefinedParams, ","))) } }