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

Enhance Condition Messages for Better Clarity and Debugging #416

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sankett45
Copy link

  1. Problem Statement

    • The status.conditions field does not fully adhere to Kubernetes conventions.
    • Condition messages are not human-readable and do not clearly convey the observed state.
    • Condition names and structure need improvements to align with Kubernetes best practices.
  2. Step 1: Standardizing Condition Types

    • Renamed condition types to follow PascalCase naming conventions.
    • Ensured condition names are concise and reflect the observed state rather than state transitions.
  3. Step 2: Improving Condition Messages

    • Modified condition messages to be clear, human-readable sentences.
    • Ensured messages provide meaningful context about the current resource state.
  4. Step 3: Refining Condition Reasons

    • Standardized condition reasons to be short, structured, and easy to understand.
    • Used concise, relevant terms such as FaultyGraph, InvalidCEL, and CRDNotSynced.
  5. Outcome

    • status.conditions now follow Kubernetes conventions.
    • Messages and reasons provide better debugging insights.
    • Condition structure is more intuitive and standardized.

@sankett45
Copy link
Author

@a-hilaly do check out this solution for issue #415 suggest any changes if any

newGraphVerifiedCondition(metav1.ConditionFalse, err.Error()),
newReconcilerReadyCondition(metav1.ConditionUnknown, "Faulty Graph"),
newCustomResourceDefinitionSyncedCondition(metav1.ConditionUnknown, "Faulty Graph"),
newGraphVerifiedCondition(metav1.ConditionFalse, "GraphValidationFailed"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need to let err.Error into the condition message?

Copy link
Contributor

@n3wscott n3wscott left a comment

Choose a reason for hiding this comment

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

Good catch! Condition Reason needs to be a Key not a free form text field.

newGraphVerifiedCondition(metav1.ConditionTrue, ""),
newCustomResourceDefinitionSyncedCondition(metav1.ConditionTrue, ""),
newReconcilerReadyCondition(metav1.ConditionFalse, err.Error()),
newGraphVerifiedCondition(metav1.ConditionTrue, "GraphValidated"),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am shocked setting reason to "" worked before, this violates the built in type validation of conditions if you use the upstream open api.

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.

2 participants