Skip to content

chore(charts): remove unused values from chart#5334

Merged
Skarlso merged 2 commits intoexternal-secrets:mainfrom
rkferreira:fix/helm-chart-unused-values
Oct 17, 2025
Merged

chore(charts): remove unused values from chart#5334
Skarlso merged 2 commits intoexternal-secrets:mainfrom
rkferreira:fix/helm-chart-unused-values

Conversation

@rkferreira
Copy link
Copy Markdown
Contributor

@rkferreira rkferreira commented Sep 18, 2025

Problem Statement

Remove unused chart values.

Related Issue

Issue 5331.

Proposed Changes

Remove:

  • certController.fullnameOverride
  • certController.nameOverride
  • webhook.fullnameOverride
  • webhook.nameOverride
  • webhook.rbac.create

Bump chart version to 0.19.3

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

@github-actions github-actions bot added area/charts Issues / Pull Requests related to our helm charts kind/chore Categorizes Pull Requests for chore activities (like bumping versions) size/s labels Sep 18, 2025
@rkferreira rkferreira mentioned this pull request Sep 18, 2025
Closed
jakobmoellerdev
jakobmoellerdev reviewed Sep 19, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

maybe stupid question but wouldnt it make sense to make the fields used instead of removing them? WDYT?

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Sep 19, 2025

maybe stupid question but wouldnt it make sense to make the fields used instead of removing them? WDYT?

Webhook certificate and other parts of it already use the global value when using the include external-secrets.fullname. So technically, these are redundant values I believe. We only have global values that are applied for all three components to keep them together.

@rkferreira
Copy link
Copy Markdown
Contributor Author

rkferreira commented Sep 19, 2025

maybe stupid question but wouldnt it make sense to make the fields used instead of removing them? WDYT?

I think the question is valid!

But, I think the best is remove it, otherwise you would have different parts of the application with totally different names.

Usually, override is just global to change this installation to some other name.

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Sep 19, 2025 •
edited
Loading

Webhook certificate and other parts of it already use the global value when using the include external-secrets.fullname. So technically, these are redundant values I believe. We only have global values that are applied for all three components to keep them together.

I can confirm these variables were introduced by myself on a WIP commit, which I never fixed (wasn't even aware they were there). Removing them is the correct path here, IMO.

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Sep 19, 2025

Though, I dont believe we should bump the chart version as a part of this PR. This should still be handled by our release process.

gusfcarvalho
gusfcarvalho reviewed Sep 19, 2025
View reviewed changes
@rkferreira
chore: remove unused values from chart
4835682 
Signed-off-by: Rodrigo Kellermann <kellermann@gmail.com>
@rkferreira rkferreira force-pushed the fix/helm-chart-unused-values branch from e5cff8a to 4835682 Compare September 19, 2025 20:50
@bharath-b-rh
Copy link
Copy Markdown
Contributor

bharath-b-rh commented Sep 22, 2025

JMHO: Should we keep the override parameters and update our charts to use them? It's handled for few resources and extending it, would be a plus for user experience, I think.

@rkferreira
Copy link
Copy Markdown
Contributor Author

rkferreira commented Sep 22, 2025

JMHO: Should we keep the override parameters and update our charts to use them? It's handled for few resources and extending it, would be a plus for user experience, I think.

I think it would increase complexity and bring little value. Unless we redesign charts, like using subcharts for components. Maybe also separate charts releases from app.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Oct 17, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Oct 17, 2025

/ok-to-test sha=741b3f6ae5a855369713d5019672a64475bda9b3

@eso-service-account-app
Copy link
Copy Markdown
Contributor

eso-service-account-app bot commented Oct 17, 2025

[Bot] - ❌ e2e for 741b3f6ae5a855369713d5019672a64475bda9b3 failed

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Oct 17, 2025

/ok-to-test sha=741b3f6ae5a855369713d5019672a64475bda9b3

@eso-service-account-app
Copy link
Copy Markdown
Contributor

eso-service-account-app bot commented Oct 17, 2025

[Bot] - ✅ e2e for 741b3f6ae5a855369713d5019672a64475bda9b3 passed

@Skarlso Skarlso merged commit ffe8fe8 into external-secrets:main Oct 17, 2025
29 checks passed
SamuelMolling pushed a commit to SamuelMolling/external-secrets that referenced this pull request Oct 24, 2025
@rkferreira @Skarlso @SamuelMolling
chore: remove unused values from chart (external-secrets#5334)
6bb9b00 
Signed-off-by: Rodrigo Kellermann <kellermann@gmail.com>
Co-authored-by: Gergely Brautigam <skarlso777@gmail.com>
Signed-off-by: Samuel Molling <samuelmolling@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gusfcarvalho gusfcarvalho gusfcarvalho left review comments

@jakobmoellerdev jakobmoellerdev jakobmoellerdev left review comments

@Skarlso Skarlso Skarlso approved these changes

Assignees

No one assigned

Labels

area/charts Issues / Pull Requests related to our helm charts kind/chore Categorizes Pull Requests for chore activities (like bumping versions) size/s

Projects

External Secrets
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rkferreira @Skarlso @gusfcarvalho @bharath-b-rh @jakobmoellerdev