HIP: Handling unknown hook-delete-policy values #403

porridge · 2025-07-23T08:54:48Z

No description provided.

robertsirc · 2025-09-25T16:33:54Z

robertsirc · 2025-09-25T16:34:40Z

can you look at this?

joejulian · 2025-09-25T18:26:30Z

hips/hip-9999.md

+#### 1. Official support for a policy which disables hook deletion
+
+Add a new, documented hook deletion policy value: `never`.
+When specified, such hook resource is not deleted.
+Effectively, the same as suggestion for Helm 3, but explicitly supported.
+
+Pros:
+- Keeps compatibility with Helm 3.
+- Allows a notion of resources which are installed by helm, but afterwards not managed by it in any way.
+- StackRox chart continues to work as before without changing.
+- Other charts (if any) which happen depend on current undocumented behaviour could easily be made to work by changing
+  whatever value they use to `never`.
+
+Cons:
+- It seems that hooks were generally intended to be resources whose previous instances are cleaned up
+  before (subsequent) chart applications. Supporting this policy breaks this assumption and
+  introduces some complexity when reasoning about possible scenarios. For example such `pre-install`
+  hook resources need to be guarded by a `if $.Release.IsInstall` condition in order not to break upgrades.


I prefer this one and would call it a bug fix. I think the con you listed, while valid, is probably the expected behavior for anyone that was previously using a not-supported string. This would just make it more explicit.

joejulian · 2025-09-25T18:26:49Z

hips/hip-9999.md

+Add a linter check that complains if an undocumented value for hook deletion policy is used.
+
+Pros:
+- Raises awareness about this issue.
+
+Cons:
+- None?


This should also be true.

scottrigby · 2025-10-09T16:54:13Z

From today's meeting per @mattfarina – this non-backwards-compatibility should be for charts API v3. charts api v2 should work the same way.

Also @porridge please change to hip-9999 to start until a number is assigned.

Signed-off-by: Marcin Owsiany <[email protected]>

- Refer to Helm charts API version, rather than Helm version. - Capitalize Helm. Signed-off-by: Marcin Owsiany <[email protected]>

porridge · 2025-10-13T08:09:45Z

From today's meeting per @mattfarina – this non-backwards-compatibility should be for charts API v3. charts api v2 should work the same way.

Also @porridge please change to hip-9999 to start until a number is assigned.

Thanks @scottrigby I changed the HIP number and references to Helm 3/4 into references to Helm charts API v2/3.

Signed-off-by: Marcin Owsiany <[email protected]>

gjenkins8 · 2025-12-04T16:59:03Z

hips/hip-9999.md

+- Other charts (if any) which happen depend on current undocumented behaviour could easily be made to work by changing
+  whatever value they use to `never`.
+
+Cons:


Does Helm apply the standard labels to hook objects?

https://helm.sh/docs/chart_best_practices/labels/#standard-labels

If so, another con is that if the chart is uninstalled. The object will remain, appearing to be managed by Helm, when that is not true.

gjenkins8 · 2025-12-04T17:12:10Z

hips/hip-9999.md

+- Other charts (if any) which happen depend on current undocumented behaviour could easily be made to work by changing
+  whatever value they use to `never`.
+
+Cons:


The docs say (for Charts API v2 of course):

Garbage collection of hook resources when the corresponding release is deleted may be added to Helm 3 in the future, so any hook resources that must never be deleted should be annotated with helm.sh/resource-policy: keep.

Another con is that never duplicates the functionality of the (currently) documented approach

This option should consider deprecating the helm.sh/resource-policy: keep policy for hooks?

gjenkins8 · 2025-12-04T17:17:49Z

hips/hip-9999.md

+- It seems that hooks were generally intended to be resources whose previous instances are cleaned up
+  before (subsequent) chart applications. Supporting this policy breaks this assumption and
+  introduces some complexity when reasoning about possible scenarios. For example such `pre-install`
+  hook resources need to be guarded by a `if $.Release.IsInstall` condition in order not to break upgrades.


pre-install hook objects are only created on install, right?

Why would a if $.Release.IsInstall condition be required to stop the object from being created on upgrade?

gjenkins8 · 2025-12-04T17:25:30Z

hips/hip-9999.md

+    for installation; StackRox operator would still use Helm library internally, and manage the storage resources differently,
+    as it does now.
+
+#### 4. Reject undocumented policies (_other than_ `never`)


This is really a sub-option of option 1? (which can choose to reject unknown policy values or not)

gjenkins8 · 2025-12-04T17:25:57Z

hips/hip-9999.md

+- If an older chart exists out there that uses an undocumented value other than `never`,
+  then its legacy versions would be incompatible with Helm charts API v3.
+
+#### 5. Warn about undocumented policies


We should do this regardless I think

(also for Charts API v2)

gjenkins8 · 2025-12-04T17:28:14Z

hips/hip-9999.md

+Disclaimer: some of the reasoning below are conjectures since the motivation is lost in the mists of time.
+
+A Helm chart was introduced for the StackRox project around 2020.
+At that time, there was a strong concern that a user mistake might lead to data loss,


Note: iirc, Helm has special logic (that I disagree with) that retains PVs upon delete

Charts API v3 should remove this IMHO

gjenkins8 · 2025-12-04T18:25:44Z

hips/hip-9999.md

+### For Helm charts API v2
+
+- No semantic changes to production code.
+- There already is a [regression test](https://github.com/helm/helm/pull/31385) to make sure that the current behaviour is not changed by mistake.


Suggested change

- There already is a [regression test](https://github.com/helm/helm/pull/31385) to make sure that the current behaviour is not changed by mistake.

- There already is a [regression test](https://github.com/helm/helm/pull/31385) (merged) to make sure that the current behaviour is not changed by mistake.

gjenkins8 · 2025-12-04T18:40:20Z

hips/hip-9999.md

+- suggests keeping status quo for Helm charts API v2, for the sake of backwards compatibility,
+- outlines a few options for charts API v3.
+
+### For Helm charts API v2


Suggested change

### For Helm charts API v2

### Charts API v2

nitpick: touch more clear I think

(and same for v3 below)

pull-request-size bot added the size/L label Jul 23, 2025

porridge mentioned this pull request Jul 23, 2025

Proposal: officially support helm.sh/hook-delete-policy: never helm/helm#13627

Closed

joejulian reviewed Sep 25, 2025

View reviewed changes

porridge added 2 commits October 13, 2025 09:58

hip-9999: Handling unknown hook-delete-policy values

372a7b2

Signed-off-by: Marcin Owsiany <[email protected]>

Update HIP index.

670df00

Signed-off-by: Marcin Owsiany <[email protected]>

porridge force-pushed the hip-27 branch from aba0208 to 670df00 Compare October 13, 2025 07:59

porridge changed the title ~~HIP 0027: Handling unknown hook-delete-policy values~~ HIP: Handling unknown hook-delete-policy values Oct 13, 2025

fix: correct helm versioning

731168a

- Refer to Helm charts API version, rather than Helm version. - Capitalize Helm. Signed-off-by: Marcin Owsiany <[email protected]>

porridge mentioned this pull request Oct 13, 2025

test: protect unknown hook delete policies helm/helm#31385

Merged

3 tasks

porridge added 2 commits October 13, 2025 10:36

Link to PR that adds the test.

b785e3e

Signed-off-by: Marcin Owsiany <[email protected]>

Mention that the regression test is merged.

ce6f8d6

Signed-off-by: Marcin Owsiany <[email protected]>

gjenkins8 reviewed Dec 4, 2025

View reviewed changes

	- There already is a [regression test](https://github.com/helm/helm/pull/31385) to make sure that the current behaviour is not changed by mistake.
	- There already is a [regression test](https://github.com/helm/helm/pull/31385) (merged) to make sure that the current behaviour is not changed by mistake.

HIP: Handling unknown hook-delete-policy values #403

Are you sure you want to change the base?

HIP: Handling unknown hook-delete-policy values #403

Uh oh!

Conversation

porridge commented Jul 23, 2025

Uh oh!

robertsirc commented Sep 25, 2025

Uh oh!

robertsirc commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottrigby commented Oct 9, 2025

Uh oh!

porridge commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gjenkins8 Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gjenkins8 Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gjenkins8 Dec 4, 2025 •

edited

Loading

gjenkins8 Dec 4, 2025 •

edited

Loading