Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve kpack conditions #1221

Merged

Conversation

evankanderson
Copy link
Contributor

Improve condition fields for kpack Images, particularly for use with Cartographer health rules:

When a matcher set is satisfied, the message field of the first condition will be replicated on the owner object.
(etc for single-condition matchers)

I also did a little cleanup, following the Kubernetes API conventions:

Conditions should always have 'reason' filled ("This field may not be empty").

Additionally, 'message' should be filled in for Unknown or Error conditions so that users know how to resolve the issue. (Inspired by a question on how to represent "build in progress" status from the tanzu apps CLI.)

Conditions should always have 'reason' filled, per https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties ("This field may not be empty").
Additionally, 'message' should be filled in for Unknown or Error conditions so that users know how to resolve the issue

Signed-off-by: Evan Anderson <evana@vmware.com>
Signed-off-by: Evan Anderson <evana@vmware.com>
func noScheduledBuild(buildNeeded corev1.ConditionStatus, builder buildapi.BuilderResource, build *buildapi.Build, sourceResolver *buildapi.SourceResolver) corev1alpha1.Conditions {
if buildNeeded == corev1.ConditionUnknown {
message := ""
func noScheduledBuild(buildNeeded buildRequiredResult, builder buildapi.BuilderResource, build *buildapi.Build, sourceResolver *buildapi.SourceResolver) corev1alpha1.Conditions {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have been overzealous here, as I had planned to include the buildRequiredResult.Reason in the status, but realized that I didn't actually need it.

@tomkennedy513
Copy link
Collaborator

Thank you for the pr @evankanderson! We will take a look

@@ -71,27 +76,49 @@ func (c *Reconciler) reconcileBuild(ctx context.Context, image *buildapi.Image,
}

func noScheduledBuild(buildNeeded corev1.ConditionStatus, builder buildapi.BuilderResource, build *buildapi.Build, sourceResolver *buildapi.SourceResolver) corev1alpha1.Conditions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function reads a little bit awkward with all the ifs that immediately return. What do you think of converting it to a switch statement to make it more explicitly layout the possible causes:

func noScheduledBuild(buildNeeded corev1.ConditionStatus, builder buildapi.BuilderResource, build *buildapi.Build, sourceResolver *buildapi.SourceResolver) corev1alpha1.Conditions {
	var (
		status  corev1.ConditionStatus
		reason  string
		message string
	)

	switch {
	case !builder.Ready():
		status = corev1.ConditionFalse
		reason = buildapi.BuilderNotReady
		message = builderError(builder)
	case buildNeeded == corev1.ConditionUnknown:
		status = corev1.ConditionUnknown
		reason = ResolverNotReadyReason
		if sourceResolver.Ready() {
			message = "Build status unknown"
		} else {
			message = fmt.Sprintf("SourceResolver %s is not ready", sourceResolver.GetName())
		}
	case !build.Status.GetCondition(corev1alpha1.ConditionSucceeded).IsTrue():
		status = unknownStatusIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded))
		reason = BuildFailedReason
		message = defaultMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded), "Last build did not succeed")
	default:
		status = unknownStatusIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded))
		reason = UpToDateReason
		message = defaultMessageIfNil(build.Status.GetCondition(corev1alpha1.ConditionSucceeded), "Last build succeeded")
	}

	return corev1alpha1.Conditions{
		{
			Type:               corev1alpha1.ConditionReady,
			Status:             status,
			Reason:             reason,
			Message:            message,
			LastTransitionTime: corev1alpha1.VolatileTime{Inner: metav1.Now()},
		},
		builderCondition(builder),
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants