Skip to content

Conversation

sjwaight
Copy link

@sjwaight sjwaight commented Jul 7, 2025

Description

The KubeFleet multi-cluster management solution requires support in Draft in order to generate ClusterResourcePlacement (CRP) definitions to place namespaces on multiple clusters. This PR adds support for generating CRP manifests in Draft.

Documentation has been updated to include the new distribute command which is based on the update command.

The feature has been delivered as an add-on.

Note: implemented through use of GitHub Copilot, with resulting implementation refined across three sessions.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Refactor - some minor refactoring to implement add-on support for Draft, with existing webapp_routing used as the baseline.

How Has This Been Tested?

Provide instructions so we can reproduce, and list any relevant details for your test configuration. Please mention if this is a breaking change which will impact consuming tool(s).

  • Unit Test:
  • E2E Test:
  • Other Test: manually tested the resulting binary build to check that it works as expected for PickAll and PickFixed CRP scenarios.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copilot AI and others added 12 commits June 27, 2025 06:37
…ariable

- Replaced two separate cluster name variables with a single comma-separated list variable
- Added custom template functions (split and trim) to parse comma-separated values
- Updated template logic to handle dynamic list of cluster names
- Updated documentation with three-cluster example as requested
- All tests updated and passing

Co-authored-by: sjwaight <[email protected]>
Add Kubefleet ClusterResourcePlacement template support and fix addon system
Resolve test failure due to add-on template name not matching expected value for webapp_routing add-on.
@sabbour sabbour requested a review from Copilot July 20, 2025 18:49
Copy link

@Copilot 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.

Pull Request Overview

This PR adds support for multi-cluster distribution by introducing a new distribute command that generates KubeFleet ClusterResourcePlacement (CRP) manifests. The implementation follows an add-on pattern similar to existing webapp routing functionality and includes comprehensive testing and documentation.

  • Introduces a new distribute command specifically for KubeFleet multi-cluster resource placement
  • Adds template functions and variable validation improvements to support conditional prompting
  • Provides comprehensive test coverage and documentation for the new functionality

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
template/addons/kubefleet/clusterresourceplacement/draft.yaml Defines template configuration for KubeFleet CRP with variables and validation
template/addons/kubefleet/clusterresourceplacement/clusterresourceplacement.yaml Template manifest for generating CRP resources with conditional cluster selection
pkg/prompts/prompts.go Moves ActiveWhen constraint checking to occur before prompt disable checks
pkg/handlers/template.go Adds custom template functions for string manipulation (split and trim)
cmd/distribute.go New command implementation for KubeFleet distribution functionality
cmd/update.go Refactors to support configurable addon templates instead of hardcoded ingress
docs/kubefleet-clusterresourceplacement.md Comprehensive documentation with usage examples and variable descriptions

- name: "PARTOF"
type: "string"
kind: "label"
description: "the label to identify which project the resource belong to"
Copy link
Preview

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

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

Grammatical error: 'belong' should be 'belongs' to match singular subject 'resource'.

Suggested change
description: "the label to identify which project the resource belong to"
description: "the label to identify which project the resource belongs to"

Copilot uses AI. Check for mistakes.

func TestWriteClusterResourcePlacementFilesExample(t *testing.T) {
err := WriteClusterResourcePlacementFilesExample()
if err != nil {
t.Errorf("WriteClusterResourcePlacementFilesExample failed: %e", err)
Copy link
Preview

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

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

Incorrect error formatting verb: '%e' is not a valid verb for error formatting. Use '%v' or '%s' instead.

Suggested change
t.Errorf("WriteClusterResourcePlacementFilesExample failed: %e", err)
t.Errorf("WriteClusterResourcePlacementFilesExample failed: %v", err)

Copilot uses AI. Check for mistakes.

// Get the kubefleet template
template, err := handlers.GetTemplate(templateType, "", outputPath, &w)
if err != nil {
return fmt.Errorf("failed to get template: %e", err)
Copy link
Preview

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

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

Incorrect error formatting verb: '%e' is not a valid verb for error formatting. Use '%v' or '%s' instead.

Suggested change
return fmt.Errorf("failed to get template: %e", err)
return fmt.Errorf("failed to get template: %v", err)

Copilot uses AI. Check for mistakes.

// Generate the ClusterResourcePlacement files
err = template.Generate()
if err != nil {
return fmt.Errorf("failed to generate manifest: %e", err)
Copy link
Preview

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

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

Incorrect error formatting verb: '%e' is not a valid verb for error formatting. Use '%v' or '%s' instead.

Suggested change
return fmt.Errorf("failed to generate manifest: %e", err)
return fmt.Errorf("failed to generate manifest: %v", err)

Copilot uses AI. Check for mistakes.

Comment on lines +16 to +18
placementType: {{ .Config.GetVariableValue "PLACEMENT_TYPE" }}{{- if eq (.Config.GetVariableValue "PLACEMENT_TYPE") "PickFixed" }}{{- $clusterNames := .Config.GetVariableValue "CLUSTER_NAMES" }}{{- if ne $clusterNames "" }}
clusterNames:{{- range (split "," $clusterNames) }}{{- $cluster := . | trim }}{{- if ne $cluster "" }}
- {{ $cluster }}{{- end }}{{- end }}{{- end }}{{- end }} No newline at end of file
Copy link
Preview

Copilot AI Jul 20, 2025

Choose a reason for hiding this comment

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

[nitpick] The complex template logic on lines 16-18 uses nested conditionals and string operations that could be difficult to debug. Consider breaking this into smaller, more readable template sections or adding comments to explain the logic flow.

Suggested change
placementType: {{ .Config.GetVariableValue "PLACEMENT_TYPE" }}{{- if eq (.Config.GetVariableValue "PLACEMENT_TYPE") "PickFixed" }}{{- $clusterNames := .Config.GetVariableValue "CLUSTER_NAMES" }}{{- if ne $clusterNames "" }}
clusterNames:{{- range (split "," $clusterNames) }}{{- $cluster := . | trim }}{{- if ne $cluster "" }}
- {{ $cluster }}{{- end }}{{- end }}{{- end }}{{- end }}
placementType: {{ .Config.GetVariableValue "PLACEMENT_TYPE" }}
{{- /* Check if placementType is "PickFixed" and process clusterNames */ -}}
{{- if eq (.Config.GetVariableValue "PLACEMENT_TYPE") "PickFixed" }}
{{- $clusterNames := .Config.GetVariableValue "CLUSTER_NAMES" }}
{{- if ne $clusterNames "" }}
clusterNames:
{{- /* Split clusterNames by comma and trim each name */ -}}
{{- range (split "," $clusterNames) }}
{{- $cluster := . | trim }}
{{- if ne $cluster "" }}
- {{ $cluster }}
{{- end }}
{{- end }}
{{- end }}
{{- end }}

Copilot uses AI. Check for mistakes.

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