Skip to content

Include generic Services in Helm v2-alpha chart generation #5256

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.

Sign up for GitHub

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

Open
UJESH2K wants to merge 2 commits into
base: master
Choose a base branch
from
+58 −0
Open

Include generic Services in Helm v2-alpha chart generation #5256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbc54fd
Include generic Services in Helm v2-alpha chart generation
UJESH2K Dec 7, 2025
cda7069
test: add generic service grouping test
UJESH2K Dec 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/generic_services_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package kustomize_test
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The test file is missing the Apache 2.0 copyright header that is present in all other test files in this directory. All test files in this package (suite_test.go, parser_test.go, chart_converter_test.go, helm_templater_test.go) include the standard copyright notice.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The package declaration uses package kustomize_test which is inconsistent with other test files in this directory. All other test files (chart_converter_test.go, helm_templater_test.go, suite_test.go) use package kustomize. While both approaches are valid in Go, consistency within a package is important for maintainability.

Suggested change
package kustomize_test
package kustomize

Copilot uses AI. Check for mistakes.

import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
kust "sigs.k8s.io/kubebuilder/v4/pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize"
)

var _ = Describe("Generic Services", func() {
It("should place non-webhook, non-metrics services into the 'services' group", func() {

// 1. Create a fake Service
service := &unstructured.Unstructured{}
service.SetKind("Service")
service.SetAPIVersion("v1")
service.SetName("my-alert-service") // does NOT contain 'metrics' or 'webhook'

// 2. Put it inside ParsedResources
parsed := &kust.ParsedResources{
Services: []*unstructured.Unstructured{service},
}

// 3. Run the ResourceOrganizer
organizer := kust.NewResourceOrganizer(parsed)
groups := organizer.OrganizeByFunction()

// 4. Expect it inside the "services" group
Expect(groups).To(HaveKey("services"))
Expect(groups["services"]).To(HaveLen(1))
Expect(groups["services"][0].GetName()).To(Equal("my-alert-service"))
})
})
24 changes: 24 additions & 0 deletions pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/resource_organizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,12 @@ func (o *ResourceOrganizer) OrganizeByFunction() map[string][]*unstructured.Unst
groups["other"] = o.resources.Other
}

// Generic Services - Services that are neither webhook nor metrics
genericServices := o.collectGenericServices()
if len(genericServices) > 0 {
groups["services"] = genericServices
}

return groups
}

Expand Down Expand Up @@ -150,6 +156,18 @@ func (o *ResourceOrganizer) collectMetricsResources() []*unstructured.Unstructur
return metricsResources
}

// collectGenericServices gathers all other service resources
func (o *ResourceOrganizer) collectGenericServices() []*unstructured.Unstructured {
var generic []*unstructured.Unstructured

for _, service := range o.resources.Services {
if o.isGenericService(service) {
generic = append(generic, service)
}
}
return generic
}
Comment on lines +159 to +169
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The new generic services functionality lacks test coverage. While the existing tests verify metrics services (lines 98-124 in chart_converter_test.go), there are no tests verifying that generic services (services without "webhook" or "metrics" in their names) are properly collected, organized into the "services" group, and written to the helm chart.

Consider adding a test case similar to the existing metrics service test that:

  1. Creates a service without "webhook" or "metrics" in its name
  2. Verifies it's written to dist/chart/templates/services directory
  3. Ensures the service is not placed in the webhook or metrics groups

Copilot uses AI. Check for mistakes.
Copy link
Author

@UJESH2K UJESH2K Dec 7, 2025

Choose a reason for hiding this comment

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

I’ve added a new unit test (generic_services_test.go) that covers the generic Service behavior mentioned here.

The test verifies that a Service without "webhook" or "metrics" in its name is:

  • detected as a generic Service

  • included in the "services" group

  • not placed under "webhook" or "metrics"

This provides test coverage for the new logic introduced in this PR.
Let me know if additional integration-level chart output tests are needed.


// collectPrometheusResources gathers prometheus related resources
func (o *ResourceOrganizer) collectPrometheusResources() []*unstructured.Unstructured {
var prometheusResources []*unstructured.Unstructured
Expand All @@ -171,3 +189,9 @@ func (o *ResourceOrganizer) isMetricsService(service *unstructured.Unstructured)
serviceName := service.GetName()
return strings.Contains(serviceName, "metrics")
}

// isGenericService determines if a service is a generic service to
// include all remaining Services that are not webhook or metrics
func (o *ResourceOrganizer) isGenericService(service *unstructured.Unstructured) bool {
return !o.isWebhookService(service) && !o.isMetricsService(service)
}
Loading