Skip to content

Conversation

@LikiosSedo
Copy link
Contributor

Motivation

This PR addresses the template field design issue discussed in KEP-8 (#70).

Problem

KEP-8 introduces templateRef and template as logically mutually exclusive options. However, with the current value-type design:

  • Zero value PodTemplateSpec{} serializes to invalid JSON that violates CRD schema
  • Users would need placeholder templates even when using templateRef (bad UX)
  • No clear way to express "template not set" vs "empty template"

Solution

Migrate Template from value type to pointer type (Solution 2 from the discussion).

Why pointer over placeholder approach:

  • Cleaner semantics: nil = not set, avoiding placeholder code smell
  • Aligns with Kubernetes conventions (large optional structs use pointers)
  • Enables strict mutual exclusion validation
  • Better foundation for future horizontal expansion (as noted by maintainers)

Timing:

  • User base is still small, best time for breaking changes
  • This PR need merge before KEP-8 implementation (as suggested by maintainers)
  • Splitting API change from KEP-8 feature for easier review

Changes

API Layer

  • RoleSpec.Template: corev1.PodTemplateSpec*corev1.PodTemplateSpec
  • Mark as +optional (was +kubebuilder:validation:Required)
  • CRD schema updated: template no longer in required fields

Controller Logic

  • Handle nil Template gracefully with zero-value fallback
  • Maintains backward compatibility: existing behavior unchanged
  • Internal functions updated to use pointer throughout

Test Updates

  • All test code updated to use pointer initialization (&template or ptr.To())
  • Approximately 10 files modified across unit tests and E2E tests

Auto-generated

  • DeepCopy code regenerated
  • CRD manifests updated

Breaking Change

For Go SDK Users

Before:

role.Template = corev1.PodTemplateSpec{
    Spec: corev1.PodSpec{
        Containers: []corev1.Container{...},
    },
}

After:
role.Template = &corev1.PodTemplateSpec{
    Spec: corev1.PodSpec{
        Containers: []corev1.Container{...},
    },
}

For YAML Users

No change required. YAML manifests remain compatible:

roles:
- name: role1
  template:  # Still works, now optional
    spec:
      containers:
      - name: app
        image: nginx

For KEP-8 templateRef mode (future PR):

 roles:
 - name: role1
   templateRef:
     name: base-template
   # template field can be omitted entirely

Testing

  • Unit tests and E2E tests verified
  • No regression in test coverage or behavior

Related

Relates to #70

This is the prerequisite API change. KEP-8 feature implementation (templateRef/templatePatch) will follow in a separate PR after community review.

  Breaking API change: RoleSpec.Template changes to pointer type.

  Changes:
  - API: Template corev1.PodTemplateSpec → *corev1.PodTemplateSpec
  - Mark field as +optional (was +kubebuilder:validation:Required)
  - Controller: Handle nil Template with zero-value fallback
  - Internal: Update patchPodTemplate to use pointer throughout
  - Tests: Update to use pointer initialization
  - Auto-generated: CRD manifests and DeepCopy code

  Breaking Change:
  - Go code must use &template or ptr.To(template)
  - YAML/JSON manifests remain compatible (template now optional)
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

LikiosSedo pushed a commit to LikiosSedo/rbg that referenced this pull request Nov 17, 2025
Apply pointer migration changes to KEP-8 branch:
- Change RoleSpec.Template from value to pointer type (*corev1.PodTemplateSpec)
- Update all test files to use pointer syntax
- Update reconcilers to handle nil Template pointer
- Update validation logic to work with pointer types
- Regenerate CRD manifests and DeepCopy code

This integrates the template pointer migration (sgl-project#102) with the
KEP-8 RoleTemplate feature, allowing Template to be optional
when using templateRef.
@RongGu RongGu requested a review from Copilot November 17, 2025 13:36
Copilot finished reviewing on behalf of RongGu November 17, 2025 13:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@RongGu
Copy link
Collaborator

RongGu commented Nov 20, 2025

/gemini review

@gemini-code-assist
Copy link

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Copilot finished reviewing on behalf of RongGu November 20, 2025 05:42
@Syspretor
Copy link
Collaborator

/lgtm
/approve

@Syspretor Syspretor merged commit f4f1452 into sgl-project:main Nov 20, 2025
18 checks passed
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.

4 participants