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 Nov 29, 2021
1 parent 2400842 commit 24d6360
Show file tree
Hide file tree
Showing 11 changed files with 67 additions and 90 deletions.
13 changes: 5 additions & 8 deletions pkg/apis/build/v1alpha1/buildrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,16 +257,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 @@ -279,7 +276,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 @@ -288,7 +285,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
15 changes: 1 addition & 14 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 @@ -175,7 +175,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 @@ -271,7 +271,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 @@ -301,7 +301,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 @@ -328,7 +328,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 @@ -370,7 +370,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 @@ -423,7 +423,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 @@ -555,7 +555,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 @@ -650,7 +650,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 @@ -714,7 +714,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 @@ -782,7 +782,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 @@ -847,7 +847,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 @@ -905,7 +905,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 @@ -942,7 +942,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 @@ -1042,7 +1042,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 @@ -125,14 +125,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 @@ -145,11 +143,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 @@ -20,7 +20,6 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/pointer"
"knative.dev/pkg/apis"
)

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
2 changes: 1 addition & 1 deletion pkg/reconciler/buildrun/resources/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,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
6 changes: 3 additions & 3 deletions test/catalog.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ func (c *Catalog) StubBuildRunStatus(reason string, name *string, condition buil
case *build.BuildRun:
if !tolerateEmptyStatus {
Expect(object.Status.GetCondition(build.Succeeded).Status).To(Equal(condition.Status))
Expect(*object.Status.GetCondition(build.Succeeded).Reason).To(Equal(*condition.Reason))
Expect(object.Status.GetCondition(build.Succeeded).Reason).To(Equal(condition.Reason))
Expect(object.Status.LatestTaskRunRef).To(Equal(name))
}
if object.Status.BuildSpec != nil {
Expand Down Expand Up @@ -895,8 +895,8 @@ func (c *Catalog) BuildRunWithSucceededCondition() *build.BuildRun {
Conditions: build.Conditions{
build.Condition{
Type: build.Succeeded,
Reason: pointer.StringPtr("foobar"),
Message: pointer.StringPtr("foo is not bar"),
Reason: "foobar",
Message: "foo is not bar",
Status: corev1.ConditionUnknown,
},
},
Expand Down
22 changes: 11 additions & 11 deletions test/integration/build_to_buildruns_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ var _ = Describe("Integration tests Build and BuildRuns", func() {
br, err := tb.GetBRTillCompletion(buildRunObject.Name)
Expect(err).To(BeNil())
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Status).To(Equal(corev1.ConditionFalse))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildRunTimeout"))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("failed to finish within"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildRunTimeout"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("failed to finish within"))
})
})

Expand All @@ -114,8 +114,8 @@ var _ = Describe("Integration tests Build and BuildRuns", func() {
br, err := tb.GetBRTillCompletion(buildRunObject.Name)
Expect(err).To(BeNil())
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Status).To(Equal(corev1.ConditionFalse))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildRunTimeout"))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("failed to finish within"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildRunTimeout"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("failed to finish within"))
})

It("should be able to override the build output", func() {
Expand Down Expand Up @@ -201,7 +201,7 @@ var _ = Describe("Integration tests Build and BuildRuns", func() {
Expect(br.Status.CompletionTime).To(BeNil())
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Type).To(Equal(v1alpha1.Succeeded))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Status).To(Equal(corev1.ConditionUnknown))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(
// BuildRun reason can be ExceededNodeResources
// if the Tekton TaskRun Pod is queued due to
// insufficient cluster resources.
Expand Down Expand Up @@ -232,8 +232,8 @@ var _ = Describe("Integration tests Build and BuildRuns", func() {
Expect(err).To(BeNil())
Expect(br.Status.StartTime).To(BeNil())
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Status).To(Equal(corev1.ConditionFalse))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildNotFound"))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("not found"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildNotFound"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("not found"))

})
})
Expand All @@ -258,8 +258,8 @@ var _ = Describe("Integration tests Build and BuildRuns", func() {
Expect(err).To(BeNil())

Expect(br.Status.GetCondition(v1alpha1.Succeeded).Status).To(Equal(corev1.ConditionFalse))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildRegistrationFailed"))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("Build is not registered correctly"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildRegistrationFailed"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("Build is not registered correctly"))
})
})

Expand Down Expand Up @@ -290,8 +290,8 @@ var _ = Describe("Integration tests Build and BuildRuns", func() {
Expect(br.Status.CompletionTime).ToNot(BeNil())
Expect(br.Status.StartTime).To(BeNil())
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Status).To(Equal(corev1.ConditionFalse))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildNotFound"))
Expect(*br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("not found"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Reason).To(Equal("BuildNotFound"))
Expect(br.Status.GetCondition(v1alpha1.Succeeded).Message).To(ContainSubstring("not found"))
})
})

Expand Down
4 changes: 2 additions & 2 deletions test/integration/buildruns_to_sa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,8 @@ var _ = Describe("Integration tests BuildRuns and Service-accounts", func() {

Expect(buildRunCondition).ToNot(BeNil())
Expect(buildRunCondition.Status).To(Equal(corev1.ConditionFalse))
Expect(*buildRunCondition.Reason).To(Equal("ServiceAccountNotFound"))
Expect(*buildRunCondition.Message).To(ContainSubstring("not found"))
Expect(buildRunCondition.Reason).To(Equal("ServiceAccountNotFound"))
Expect(buildRunCondition.Message).To(ContainSubstring("not found"))
})
})
})
Loading

0 comments on commit 24d6360

Please sign in to comment.