Skip to content

Commit

Permalink
Change optional fields in inside condition to required
Browse files Browse the repository at this point in the history
  • Loading branch information
Shahul authored and Shahul committed Jan 21, 2022
1 parent c199165 commit 6f28bb2
Show file tree
Hide file tree
Showing 15 changed files with 92 additions and 104 deletions.
3 changes: 3 additions & 0 deletions deploy/crds/shipwright.io_buildruns.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,9 @@ spec:
description: Type of condition
type: string
required:
- lastTransitionTime
- message
- reason
- status
- type
type: object
Expand Down
19 changes: 8 additions & 11 deletions pkg/apis/build/v1alpha1/buildrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type BuildRunSpec struct {
// (`.spec.source`) data.
//
// +optional
Sources *[]BuildSource `json:"sources,omitempty"`
Sources []BuildSource `json:"sources,omitempty"`

// ServiceAccount refers to the kubernetes serviceaccount
// which is used for resource control.
Expand Down Expand Up @@ -131,12 +131,12 @@ type BuildRunStatus struct {
// of different sources
//
// +optional
Sources []SourceResult `json:"sources"`
Sources []SourceResult `json:"sources,omitempty"`

// Output holds the results emitted from step definition of an output
//
// +optional
Output *Output `json:"output"`
Output *Output `json:"output,omitempty"`

// Conditions holds the latest available observations of a resource's current state.
Conditions Conditions `json:"conditions,omitempty"`
Expand Down Expand Up @@ -274,16 +274,13 @@ type Condition struct {
Status corev1.ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"`

// 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" 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" 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" description:"human-readable message indicating details about last transition"`
}

func init() {
Expand All @@ -296,7 +293,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
Expand All @@ -305,7 +302,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
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/build/v1alpha1/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type Source struct {
// BundleContainer
//
// +optional
BundleContainer *BundleContainer `json:"bundleContainer"`
BundleContainer *BundleContainer `json:"bundleContainer,omitempty"`

// Revision describes the Git revision (e.g., branch, tag, commit SHA,
// etc.) to fetch.
Expand Down
26 changes: 11 additions & 15 deletions pkg/apis/build/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 14 additions & 14 deletions pkg/reconciler/buildrun/buildrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ var _ = Describe("Reconcile BuildRun", func() {
&taskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("Succeeded"),
Reason: "Succeeded",
Status: corev1.ConditionTrue,
},
corev1.ConditionTrue,
Expand Down Expand Up @@ -267,7 +267,7 @@ var _ = Describe("Reconcile BuildRun", func() {
&taskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("Pending"),
Reason: "Pending",
Status: corev1.ConditionUnknown,
},
corev1.ConditionUnknown,
Expand Down Expand Up @@ -297,7 +297,7 @@ var _ = Describe("Reconcile BuildRun", func() {
&taskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("Running"),
Reason: "Running",
Status: corev1.ConditionUnknown,
},
corev1.ConditionUnknown,
Expand All @@ -324,7 +324,7 @@ var _ = Describe("Reconcile BuildRun", func() {
&taskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("Succeeded"),
Reason: "Succeeded",
Status: corev1.ConditionTrue,
},
corev1.ConditionTrue,
Expand Down Expand Up @@ -366,7 +366,7 @@ var _ = Describe("Reconcile BuildRun", func() {
switch v := object.(type) {
case *build.BuildRun:
c := v.Status.GetCondition(build.Succeeded)
if c != nil && *c.Reason == build.BuildRunStateCancel && c.Status == corev1.ConditionFalse {
if c != nil && c.Reason == build.BuildRunStateCancel && c.Status == corev1.ConditionFalse {
cancelUpdateCalled = true
}

Expand Down Expand Up @@ -419,7 +419,7 @@ var _ = Describe("Reconcile BuildRun", func() {
&taskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("something bad happened"),
Reason: "something bad happened",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down Expand Up @@ -551,7 +551,7 @@ var _ = Describe("Reconcile BuildRun", func() {
switch v := object.(type) {
case *build.BuildRun:
c := v.Status.GetCondition(build.Succeeded)
if c != nil && *c.Reason == build.BuildRunStateCancel {
if c != nil && c.Reason == build.BuildRunStateCancel {
cancelUpdateCalled = true
}

Expand Down Expand Up @@ -646,7 +646,7 @@ var _ = Describe("Reconcile BuildRun", func() {
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("ServiceAccountNotFound"),
Reason: "ServiceAccountNotFound",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down Expand Up @@ -710,7 +710,7 @@ var _ = Describe("Reconcile BuildRun", func() {
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("BuildStrategyNotFound"),
Reason: "BuildStrategyNotFound",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down Expand Up @@ -778,7 +778,7 @@ var _ = Describe("Reconcile BuildRun", func() {
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("ClusterBuildStrategyNotFound"),
Reason: "ClusterBuildStrategyNotFound",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down Expand Up @@ -843,7 +843,7 @@ var _ = Describe("Reconcile BuildRun", func() {
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("UnknownStrategyKind"),
Reason: "UnknownStrategyKind",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down Expand Up @@ -901,7 +901,7 @@ var _ = Describe("Reconcile BuildRun", func() {
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("BuildStrategyNotFound"),
Reason: "BuildStrategyNotFound",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down Expand Up @@ -938,7 +938,7 @@ var _ = Describe("Reconcile BuildRun", func() {
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("SetOwnerReferenceFailed"),
Reason: "SetOwnerReferenceFailed",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down Expand Up @@ -1037,7 +1037,7 @@ var _ = Describe("Reconcile BuildRun", func() {
emptyTaskRunName,
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("BuildRegistrationFailed"),
Reason: "BuildRegistrationFailed",
Status: corev1.ConditionFalse,
},
corev1.ConditionFalse,
Expand Down
14 changes: 6 additions & 8 deletions pkg/reconciler/buildrun/resources/conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,12 @@ func UpdateBuildRunUsingTaskRunCondition(ctx context.Context, client client.Clie
}
}

lastTransitionTime := metav1.Now()

buildRun.Status.SetCondition(&buildv1alpha1.Condition{
LastTransitionTime: &lastTransitionTime,
LastTransitionTime: metav1.Now(),
Type: buildv1alpha1.Succeeded,
Status: status,
Reason: &reason,
Message: &message,
Reason: reason,
Message: message,
})

return nil
Expand All @@ -129,11 +127,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 {
Expand Down
18 changes: 7 additions & 11 deletions pkg/reconciler/buildrun/resources/conditions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/pointer"
"knative.dev/pkg/apis"
crc "sigs.k8s.io/controller-runtime/pkg/client"
)
Expand Down Expand Up @@ -69,14 +68,13 @@ var _ = Describe("Conditions", func() {

It("should be able to set a condition based on a type", func() {
br := ctl.DefaultBuildRun("foo", "bar")
lastTransitionTime := metav1.Now()
// generate a condition of the type Succeeded
tmpCond := &build.Condition{
Type: build.Succeeded,
Status: corev1.ConditionUnknown,
Message: pointer.StringPtr("foobar"),
Reason: pointer.StringPtr("foo is bar"),
LastTransitionTime: &lastTransitionTime,
Message: "foobar",
Reason: "foo is bar",
LastTransitionTime: metav1.Now(),
}

// set the condition on the BuildRun resource
Expand All @@ -97,15 +95,13 @@ var _ = Describe("Conditions", func() {
reason := br.Status.GetCondition(build.Succeeded).GetReason()
Expect(reason).To(Equal("foobar"))

lastTransitionTime := metav1.Now()

// generate a condition in order to update the existing one
tmpCond := &build.Condition{
Type: build.Succeeded,
Status: corev1.ConditionUnknown,
Message: pointer.StringPtr("foobar was updated"),
Reason: pointer.StringPtr("foo is bar"),
LastTransitionTime: &lastTransitionTime,
Message: "foobar was updated",
Reason: "foo is bar",
LastTransitionTime: metav1.Now(),
}

// update the condition on the BuildRun resource
Expand Down Expand Up @@ -280,7 +276,7 @@ var _ = Describe("Conditions", func() {
)).To(BeNil())

// Finally, check the output of the buildRun
Expect(*br.Status.GetCondition(
Expect(br.Status.GetCondition(
build.Succeeded).Reason,
).To(Equal(build.BuildRunStatePodEvicted))
})
Expand Down
6 changes: 3 additions & 3 deletions pkg/reconciler/buildrun/resources/sources.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ func isLocalCopyBuildSource(
if build.Spec.Sources != nil {
sources = append(sources, build.Spec.Sources...)
}
if buildRun.Spec.Sources != nil {
sources = append(sources, *buildRun.Spec.Sources...)
}

sources = append(sources, buildRun.Spec.Sources...)

for _, source := range sources {
if source.Type == buildv1alpha1.LocalCopy {
return &source
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/buildrun/resources/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ func GenerateTaskRun(
},
})
}
if build.Spec.Dockerfile != nil {
if build.Spec.Dockerfile != nil && *build.Spec.Dockerfile != "" {
params = append(params, v1beta1.Param{
Name: inputParamDockerfile,
Value: v1beta1.ArrayOrString{
Expand Down
2 changes: 1 addition & 1 deletion pkg/validate/sourceurl.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type SourceURLRef struct {
// that the spec.source.url exists. This validation only applies
// to endpoints that do not require authentication.
func (s SourceURLRef) ValidatePath(ctx context.Context) error {
if s.Build.Spec.Source.Credentials == nil {
if s.Build.Spec.Source.Credentials == nil && s.Build.Spec.Source.URL != nil {
switch s.Build.GetAnnotations()[build.AnnotationBuildVerifyRepository] {
case "true":
if err := git.ValidateGitURLExists(ctx, *s.Build.Spec.Source.URL); err != nil {
Expand Down
Loading

0 comments on commit 6f28bb2

Please sign in to comment.