🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks #4584

camilamacedo86 · 2025-02-28T09:27:24Z

Closes: #4582

k8s-ci-robot · 2025-02-28T09:27:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sbin64 · 2025-03-03T08:27:09Z

pkg/plugins/optional/helm/v1alpha/scaffolds/init.go

@@ -96,7 +97,7 @@ func (s *initScaffolder) Scaffold() error {
 			DeployImages: len(imagesEnvVars) > 0,
 			HasWebhooks:  hasWebhooks,
 		},
-		&templatescertmanager.Certificate{},
+		&templatescertmanager.Certificate{HasWebhooks: hasWebhooks},


sorry Camila, just wondered whether it is being updated due to scaffalding webhooks conditional issues.

we are passing the value HasWebhooks here because then inside of the boilerplate to generate the Certs we just generate those which related to Webhook Sever only when we have Webhooks

sbin64

Looks good, but why this has failed. I suspect from here whether intended.

sigs.k8s.io/kubebuilder/v4/pkg/config
  Incompatible changes:
  - Config.HasWebhooks: added

camilamacedo86 · 2025-03-07T08:44:54Z

Regards: #4584 (review)
The lib try to help us know about breaking changes in the API
In this case, shows a false alarm. We are adding a new method, not removing.

We can either do not change the API and keep the implementation inside of the plugin.
It might be a better choice, if we see more places that needs it then we can consider in expose the method

camilamacedo86 · 2025-03-07T08:47:15Z

pkg/config/v3/config.go

@@ -172,6 +172,23 @@ func (c Cfg) HasResource(gvk resource.GVK) bool {
 	return found
 }

+// HasWebhooks checks if any stored resource has associated webhooks.
+func (c Cfg) HasWebhooks() bool {


/hold

Remove from the interface

camilamacedo86 · 2025-03-19T10:49:41Z

/hold cancel

It is good to fly now.

Kavinjsir

Hey @camilamacedo86, everything’s looking great! I just dropped a few tiny concerns.

Kavinjsir · 2025-03-22T21:41:24Z

...lugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/manager/manager.go

      {{ "{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}" }}
+{{- else }}
+      {{ "{{- if and .Values.certmanager.enable .Values.metrics.enable }}" }}
+{{- end }}


Nit:
For the following two lines, how would you like we predefine variables as references for code readability?

{{ "{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}" }}

{{ "{{- if and .Values.certmanager.enable .Values.metrics.enable }}" }}

Such as:

{{- $certmanagerConditionWithHooks := `{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}` -}} {{- $certmanagerConditionNoHooks := `{{- if and .Values.certmanager.enable .Values.metrics.enable }}` -}} {{- if .HasWebhooks }} {{ $certmanagerConditionWithHooks }} {{- else }} {{ $certmanagerConditionNoHooks }} {{- end }}

Or perhaps, having these values defined in helpers.tpl?

I'm not sure if that would work well — we'd need to test it out first.
Since we're generating boilerplate to produce a template, it's a bit tricky to ensure consistent outputs.

Also, the certmanagerConditionWithHooks and certmanagerConditionNoHooks shouldn't be in the helpers.tpl. The idea is to only include a Helm template with .Values.webhook.enable when the project actually uses webhooks.

That said, if you'd like, we can definitely explore making changes in a follow-up.
If you see a clear path to improve things, we’ll likely need to update other parts as well to keep everything consistent and aligned with the same approach.

So for now, I think we can move forward with the current fix — but please feel free to open issues, PRs, etc., so we can address improvements more broadly.

Kavinjsir · 2025-03-22T21:49:24Z

pkg/plugins/optional/helm/v1alpha/scaffolds/init.go

 	mutatingWebhooks, validatingWebhooks, err := s.extractWebhooksFromGeneratedFiles()
 	if err != nil {
 		return fmt.Errorf("failed to extract webhooks: %w", err)
 	}
+	hasWebhooks := hasWebhooks(s.config) || (len(mutatingWebhooks) > 0 && len(validatingWebhooks) > 0)


Wondering if it’d be okay to change the name of the boolean variable so it’s not the same as the function hasWebhooks
Or, alternatively, update the function name instead, maybe something like webhooksConfigured.
(Just in case this would help readability.)

Thank you.
I changed the func name for hasWebhooksWith which seems the most appropriate.

… with webhook conditions when webhooks are not used

camilamacedo86 · 2025-03-25T12:41:34Z

HI @Kavinjsir

I think we can merge this since it fixes the issues, looks for improvements, etc, in follow-up, wdyt?
Do we have your LGTM? PS. Your recommendation about having variables and functions with different names is addressed now, too.

Thank youvery mucht for your time.

camilamacedo86 · 2025-03-26T14:02:44Z

Since the comments have been addressed and there were no objections to the fix, I'm going ahead and merging this so we can get it into the next release.

That said, we can always iterate and improve on it in a follow-up as needed.

Please feel free to open new PRs if you have suggestions or improvements in mind — always happy to review!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 28, 2025

k8s-ci-robot requested a review from Kavinjsir February 28, 2025 09:27

k8s-ci-robot requested a review from varshaprasad96 February 28, 2025 09:27

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 28, 2025

camilamacedo86 force-pushed the fix-helm-no-webhook branch from 2532ad1 to 44121b9 Compare February 28, 2025 10:26

camilamacedo86 changed the title ~~🐛 (helm/v1-alpha): Fix helm-chart scaffold by not scaffolding manifests with webhook conditions when webhooks are not used~~ 🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks Feb 28, 2025

camilamacedo86 force-pushed the fix-helm-no-webhook branch from 44121b9 to e1333e2 Compare February 28, 2025 13:14

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 28, 2025

camilamacedo86 mentioned this pull request Feb 28, 2025

(helm/v1-alpha) Generates invalid chart in operator without webhooks #4582

Closed

sbin64 reviewed Mar 3, 2025

View reviewed changes

camilamacedo86 requested a review from sbin64 March 6, 2025 22:56

sbin64 reviewed Mar 7, 2025

View reviewed changes

camilamacedo86 commented Mar 7, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2025

camilamacedo86 force-pushed the fix-helm-no-webhook branch from e1333e2 to 949225f Compare March 19, 2025 10:48

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2025

camilamacedo86 force-pushed the fix-helm-no-webhook branch from 949225f to 0bbf858 Compare March 19, 2025 11:33

Kavinjsir reviewed Mar 22, 2025

View reviewed changes

(helm/v1-alpha): Fix helm-chart scaffold by not scaffolding manifests…

8e0dc31

… with webhook conditions when webhooks are not used

camilamacedo86 force-pushed the fix-helm-no-webhook branch from 0bbf858 to 8e0dc31 Compare March 25, 2025 12:40

camilamacedo86 requested review from Kavinjsir and sbin64 March 25, 2025 12:41

camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 26, 2025

camilamacedo86 merged commit d90d095 into kubernetes-sigs:master Mar 26, 2025
24 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks #4584

🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks #4584

camilamacedo86 commented Feb 28, 2025 •

edited

Loading

k8s-ci-robot commented Feb 28, 2025

sbin64 Mar 3, 2025

camilamacedo86 Mar 3, 2025

sbin64 left a comment

camilamacedo86 commented Mar 7, 2025 •

edited

Loading

camilamacedo86 Mar 7, 2025

camilamacedo86 commented Mar 19, 2025

Kavinjsir left a comment

Kavinjsir Mar 22, 2025 •

edited

Loading

camilamacedo86 Mar 25, 2025

Kavinjsir Mar 22, 2025

camilamacedo86 Mar 25, 2025

camilamacedo86 commented Mar 25, 2025

camilamacedo86 commented Mar 26, 2025

🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks #4584

🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks #4584

Conversation

camilamacedo86 commented Feb 28, 2025 • edited Loading

k8s-ci-robot commented Feb 28, 2025

sbin64 Mar 3, 2025

Choose a reason for hiding this comment

camilamacedo86 Mar 3, 2025

Choose a reason for hiding this comment

sbin64 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Mar 7, 2025 • edited Loading

camilamacedo86 Mar 7, 2025

Choose a reason for hiding this comment

camilamacedo86 commented Mar 19, 2025

Kavinjsir left a comment

Choose a reason for hiding this comment

Kavinjsir Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 Mar 25, 2025

Choose a reason for hiding this comment

Kavinjsir Mar 22, 2025

Choose a reason for hiding this comment

camilamacedo86 Mar 25, 2025

Choose a reason for hiding this comment

camilamacedo86 commented Mar 25, 2025

camilamacedo86 commented Mar 26, 2025

camilamacedo86 commented Feb 28, 2025 •

edited

Loading

camilamacedo86 commented Mar 7, 2025 •

edited

Loading

Kavinjsir Mar 22, 2025 •

edited

Loading