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
}
Copy link
Member

@camilamacedo86 camilamacedo86 Dec 10, 2025

Choose a reason for hiding this comment

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

Probably instead of allow only any service we might want to migrate any kustomize scaffolds, but I think it is the case already. So, could we please check because as it is now I think we are already creating the Helm Chart with all that is in the kustomize install.yaml if not then I think we need a more generic solution. WDYT?

Copy link
Author

@UJESH2K UJESH2K Dec 13, 2025

Choose a reason for hiding this comment

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

Thanks for the clarification — I agree.
After rechecking, we’re not currently porting all resources from install.yaml into the Helm chart; only explicitly handled kinds are included, while others are silently ignored.
I agree that adding support only for generic Services creates inconsistent behavior if other resource kinds are still dropped. A generic mechanism to port any non-scaffolded resource would be cleaner than special-casing Services.
I’m happy to refactor this toward a generic solution if that aligns with the project direction. Please let me know how you’d like to proceed.

Copy link
Member

@camilamacedo86 camilamacedo86 Dec 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The idea gave for the folk that report this one to add all others one in a generic dir like others might be fit.
I think we may can:

  • After all check if has any resource in the install.yaml that was not ported to the helm chart
  • Then port to a generic dir like others
  • Then warning something like "resources that were not identified were added under template/extras"

Something like might be a good approach
open to ideas and suggestions as well


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.

Copy link
Member

@camilamacedo86 camilamacedo86 Dec 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

OK, that makes sense. However, if we do this for Services, we should do it for any kind of resource.

Consistent behavior is key. We cannot port some resources (like Services) while silently ignoring others.

So we either need:

  • a generic way to port all resources from install.yaml that are not part of the default scaffold, or
  • to not port any of them and clearly communicate that limitation.

Partial support would be confusing and hard to maintain.

Related discussion:
#5248 (comment)


// 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