Fix interactive apply asks in no change situation by tarrow · Pull Request #945 · helmfile/helmfile

tarrow · 2023-07-25T10:48:54Z

This commit makes the apply logic exit early in the event there are no
changes to releases. I believe this effectively reverts #522.

Updates relevant snapshot

Fixes: #679

yxxhero · 2023-07-26T01:03:26Z

@tarrow if when preapply hook changes something. do we need to ask?

tarrow · 2023-07-26T09:33:09Z

@yxxhero An excellent question; I'm not certain but my belief is "no".

I do not use the preapply hooks but my understanding from https://helmfile.readthedocs.io/en/latest/#hooks is that they are intended to be used for two types of different things:

to interact with the cluster in an idempotent way e.g. to install additional resources before the release is installed
to print contextual information to the user

In the case of 1 there is an argument that user's may expect to have the opportunity to confirm this action. However it seems never to have been the case that the hooks that would be executed are displayed to the user. In this case the user has no idea what they are agreeing to (unlike creating, updating or deleting releases which are printed before hand). If this feature is desired I would probably recommend adding an additional flag (ask-before-hook or something?)

In case 2 it seems pretty clear that they would want this contextual information printed before being given the decision to make; in this case I believe this patch is an improvement for the user experience

yxxhero · 2023-07-26T13:18:27Z

@mumoshu WDYT?

tarrow · 2023-07-31T10:49:33Z

Please let me know if you require anything more from me; if you determine that some other behaviour is desired let me know and I may be able to update the PR in order to facilitate that

tarrow · 2023-08-14T17:15:21Z

@yxxhero and @mumoshu can I do anything to help unblock this?

yxxhero · 2023-08-14T23:35:13Z

@mumoshu ping

mumoshu · 2023-08-16T00:10:33Z

 	// Traverse DAG of all the releases so that we don't suffer from false-positive missing dependencies
 	st.Releases = selectedAndNeededReleases

-	if !interactive || interactive && r.askForConfirmation(confMsg) {


@tarrow Hey! Thanks for your contribution. I was trying to understand the original issue and your change fully but had no luck so far unfortunately.. .so it would be great if you could clarify a bit!

In short, can't we just inject something like the below here?

if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 { return true, false, nil }

(basically L1498-1501)

That way, I think we can fix the original issue without changing the behavior of preapply hooks.

I might have missed something but my mind says preapply hooks should run immediately before applying. Running it before prompting breaks that rule so I'm slightly inclined to not change it's behaviour this way.

BTW, do you have any motivation that you want some hooks to run even before prompting? We could add a new hook point for that, perhaps something like preprompt.

Alterntively, you could just use the existing prepare hook if you want some hooks to be run regardless of if there is any change to be applied.

Hi @mumoshu! Thanks for the reply; sorry for the slow response.

In short, can't we just inject something like the below here?

Yes! I think we can; I'd be very happy with this solution if you are. I have uploaded an updated patch with this change.

For clarity: I don't use any of the pre-apply hooks. I was actually trying to minimise behaviour change.

I noticed in #522 you said that

preapply is supposed to get triggered even on unmodified releases

I was trying to keep this behaviour. With this new patchset I believe that this will no longer be true. preapply will now not be triggered if there are no unmodified releases. I added this to the commit message any updated the docs. Please do double check that you're happy with this since I don't understand if this change is desired (since I don't use preapply hooks).

stale · 2023-09-03T08:45:19Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2023-09-19T19:56:23Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sjentzsch · 2023-09-19T20:33:47Z

@mumoshu could you take a look at the review once more? 🙏

stale · 2023-10-03T22:41:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sjentzsch · 2023-10-03T22:46:48Z

ping

tarrow · 2023-10-04T09:10:04Z

Thanks for the pings to keep this alive. To make it clear the TestApply_hooks/hooks_for_no-diff_release test (and the noop) failing was left deliberately by me to make it clear the implications of the adjusted change.

Please do let me know what you think about it; happy to make adjustments or just continue the discussion about what is the most desirable behaviour.

stale · 2023-10-18T10:38:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sjentzsch · 2023-10-18T10:45:09Z

ping @mumoshu

tarrow · 2026-01-23T15:08:33Z

Seems like there were more snapshots that are either new I missed relating to pre-apply hooks. I also updated these. Can someone approve the workflows again please ?

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Copilot · 2026-01-24T00:42:45Z


 `preapply` hooks are triggered before a release is uninstalled, installed, or upgraded as part of `helmfile apply`.
-This is the ideal event to hook into when you are going to use `helmfile apply` for every kind of change and you want the hook to be triggered regardless of whether the releases have changed or not. Be sure to make each `preapply` hook command idempotent. Otherwise, rerunning helmfile-apply on a transient failure may end up either breaking your cluster, or the hook that runs for the second time will never succeed.
+This is the ideal event to hook into when you are going to use `helmfile apply` for every kind of change and it is acceptable for the hook to be triggered regardless of whether the releases have changed or not (n.b. if there are no changes for any releases then this will not run). Be sure to make each `preapply` hook command idempotent. Otherwise, rerunning helmfile-apply on a transient failure may end up either breaking your cluster, or the hook that runs for the second time will never succeed.


The documentation is self-contradictory. It first states "it is acceptable for the hook to be triggered regardless of whether the releases have changed or not" but then immediately says "(n.b. if there are no changes for any releases then this will not run)". Consider rewriting this to be clearer. For example: "This is the ideal event to hook into when you are going to use helmfile apply for every kind of change. Note that preapply hooks will only run if at least one release has changes to apply. Be sure to make each preapply hook command idempotent. Otherwise, rerunning helmfile-apply on a transient failure may end up either breaking your cluster, or the hook that runs for the second time will never succeed."

Suggested change

This is the ideal event to hook into when you are going to use `helmfile apply` for every kind of change and it is acceptable for the hook to be triggered regardless of whether the releases have changed or not (n.b. if there are no changes for any releases then this will not run). Be sure to make each `preapply` hook command idempotent. Otherwise, rerunning helmfile-apply on a transient failure may end up either breaking your cluster, or the hook that runs for the second time will never succeed.

This is the ideal event to hook into when you are going to use `helmfile apply` for every kind of change. Note that `preapply` hooks will only run if at least one release has changes to apply. Be sure to make each `preapply` hook command idempotent. Otherwise, rerunning `helmfile apply` on a transient failure may end up either breaking your cluster, or the hook that runs for the second time will never succeed.

yxxhero · 2026-01-25T03:25:37Z

Code Review

I've reviewed this PR and have the following observations:

Summary

This PR adds an early exit for helmfile apply when there are no changes, which effectively fixes issue #679 by preventing unnecessary confirmation prompts.

Code Changes

The implementation in pkg/app/app.go:1637-1641 is clean and minimal:

if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 {
    return true, false, nil
}

This correctly exits before the interactive prompt and before preapply hooks run.

Concerns

1. Breaking change to preapply hooks

This PR reverts behavior from Fix preapply hooks are not called on no diff when run apply subcommand #522 where preapply hooks ran even on unmodified releases
Test snapshots confirm hooks no longer execute when there are no changes
May impact users who rely on preapply hooks running unconditionally (e.g., validation, reporting)

2. Documentation contradiction
As noted by Copilot AI, the docs contain contradictory language:

States: "it is acceptable for the hook to be triggered regardless of whether the releases have changed or not"
Then says: "if there are no changes for any releases then this will not run"

3. Migration considerations

Users may need to migrate preapply hooks to prepare hooks if they need unconditional execution
Consider documenting this breaking change in release notes

Recommendation

Approve with conditions:

✅ Fix documentation to be clear and non-contradictory
⚠️ Consider adding a new hook point (e.g., preprompt) for users who need hooks before confirmation
📝 Add migration notes about the preapply behavior change

The implementation correctly addresses issue #679 and is technically sound. The main blockers are documentation clarity and confirming the team is comfortable with the preapply behavior change.

Overall: LGTM with documentation fix

yxxhero · 2026-01-25T03:25:44Z

@tarrow The documentation change in docs/index.md still contains contradictory language that should be fixed before merge.

Current text:

This is the ideal event to hook into when you are going to use helmfile apply for every kind of change and it is acceptable for the hook to be triggered regardless of whether the releases have changed or not (n.b. if there are no changes for any releases then this will not run).

This is self-contradictory. I suggest rewriting to:

This is the ideal event to hook into when you are going to use helmfile apply for every kind of change. Note that preapply hooks will only run if at least one release has changes to apply. Be sure to make each preapply hook command idempotent. Otherwise, rerunning helmfile apply on a transient failure may end up either breaking your cluster, or the hook that runs for the second time will never succeed.

This is clearer and accurately reflects the new behavior introduced by this PR.

yxxhero · 2026-01-25T03:25:51Z

@mumoshu This PR has been waiting for your final approval since September 2023. The implementation correctly addresses issue #679 and follows your suggestion from your review comment on Aug 16, 2023.

Would you be able to provide approval or any final feedback needed to move this forward? The community is eagerly awaiting this fix - see comments from @outdooracorn and others.

Thanks!

tarrow · 2026-01-26T21:57:10Z

@yxxhero Thanks for nudging this along! I've adjusted the docs as you requested from your copilot review

yxxhero · 2026-01-26T23:59:01Z

@tarrow fix DCO issue.

tarrow · 2026-01-28T10:39:43Z

@yxxhero hope you're happy with the DCO and the message with it. I tried to make it clear that I didn't write the docs change in the commit message. Happy for you to also add a DCO for that bit if you like :)

tarrow · 2026-02-06T14:47:42Z

@yxxhero how's it looking from your side? Do you need anything further from me?

yxxhero · 2026-02-12T03:42:35Z

@tarrow no. I will review this again.

yxxhero

Review Summary

Thanks for the fix! This addresses the reported issue in #679 where helmfile apply -i prompts for confirmation even when no changes exist. The approach follows the maintainer's suggestion and is minimal.

Concerns

1. Breaking Change for preapply hooks ⚠️

The documentation update indicates that preapply hooks "will only run if at least one release has changes to apply". This is a breaking change from the original design established in #522, where preapply was explicitly designed to run "regardless of whether the releases have changed or not".

Users relying on preapply hooks for side-effects (e.g., notifications, audits, metric collection) will no longer see those hooks fire when there are no changes. This should be:

Called out explicitly in release notes / changelog
Considered if this is the intended behavior long-term

2. Test Coverage

The test changes are snapshot updates only. Consider adding an explicit unit test that verifies:

No prompt is shown when there are no changes
Preapply hooks don't run when there are no changes
Preapply hooks still run when at least one release has changes

3. Rebase Needed

The PR appears to be based on an older version of main. The code structure in the diff doesn't match the current main branch. Please rebase onto the latest main before merge to ensure:

Clean merge
All tests pass with current code
No conflicts with recent changes

Minor Nit

The docs phrasing is good now after the Copilot suggestion. The current text is clear about the new behavior.

Overall: The fix is sound. After addressing the rebase and considering the breaking change communication, this should be ready to merge.

yxxhero · 2026-03-09T23:45:33Z

@tarrow please fix conflicts.

This commit makes the apply logic exit early in the event there are no changes to releases. I believe this effectively reverts helmfile#522. Updates relevant snapshots Clarify conditions under which preapply hooks are triggered to include that they will no longer fire if there is a no-op. Docs as requested by the maintainer from a copilot request made by them. Fixes: helmfile#679 Signed-off-by: Thomas Arrow <[email protected]>

tarrow · 2026-03-10T15:16:11Z

@yxxhero rebase done I hope! Could you you please approve the test jobs again? :)

tarrow · 2026-03-18T10:43:08Z

Wonderful! Do you need anything more from me to merge this?

tarrow requested review from itscaro, mumoshu, xiaomudk and yxxhero as code owners July 25, 2023 10:48

tarrow force-pushed the ta/fixInteractiveOnNoChange branch from 311c46b to 326113f Compare July 25, 2023 10:49

mumoshu reviewed Aug 16, 2023

View reviewed changes

stale Bot added the wontfix This will not be worked on label Sep 3, 2023

yxxhero removed the wontfix This will not be worked on label Sep 3, 2023

tarrow force-pushed the ta/fixInteractiveOnNoChange branch 2 times, most recently from 44541d2 to 9bbad20 Compare September 4, 2023 13:19

stale Bot added the wontfix This will not be worked on label Sep 19, 2023

stale Bot removed the wontfix This will not be worked on label Sep 19, 2023

stale Bot added the wontfix This will not be worked on label Oct 3, 2023

stale Bot removed the wontfix This will not be worked on label Oct 3, 2023

stale Bot added the wontfix This will not be worked on label Oct 18, 2023

stale Bot removed the wontfix This will not be worked on label Oct 18, 2023

tarrow force-pushed the ta/fixInteractiveOnNoChange branch from beb15ea to fcc0d4d Compare January 23, 2026 15:07

yxxhero requested a review from Copilot January 24, 2026 00:36

Copilot started reviewing on behalf of yxxhero January 24, 2026 00:37 View session

Copilot AI reviewed Jan 24, 2026

View reviewed changes

tarrow force-pushed the ta/fixInteractiveOnNoChange branch from fc85cda to d87f011 Compare January 28, 2026 10:36

yxxhero requested changes Feb 12, 2026

View reviewed changes

yxxhero force-pushed the ta/fixInteractiveOnNoChange branch from d87f011 to 7bbeb10 Compare February 12, 2026 03:59

tarrow force-pushed the ta/fixInteractiveOnNoChange branch from 7bbeb10 to 61cdfe9 Compare March 10, 2026 14:29

tarrow requested a review from aditmeno as a code owner March 10, 2026 14:29

tarrow force-pushed the ta/fixInteractiveOnNoChange branch from 61cdfe9 to 12c545f Compare March 10, 2026 15:15

Merge branch 'main' into ta/fixInteractiveOnNoChange

19003ed

yxxhero self-requested a review March 18, 2026 01:21

yxxhero approved these changes Mar 18, 2026

View reviewed changes

yxxhero merged commit 06d0994 into helmfile:main Mar 19, 2026
16 checks passed

BrewTestBot mentioned this pull request Mar 24, 2026

helmfile 1.4.3 Homebrew/homebrew-core#273949

Merged

Uh oh!

Uh oh!

Conversation

tarrow commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yxxhero commented Jul 26, 2023

Uh oh!

tarrow commented Jul 26, 2023

Uh oh!

yxxhero commented Jul 26, 2023

Uh oh!

tarrow commented Jul 31, 2023

Uh oh!

tarrow commented Aug 14, 2023

Uh oh!

yxxhero commented Aug 14, 2023

Uh oh!

mumoshu Aug 16, 2023 • 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!

tarrow Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Sep 3, 2023

Uh oh!

stale Bot commented Sep 19, 2023

Uh oh!

sjentzsch commented Sep 19, 2023

Uh oh!

stale Bot commented Oct 3, 2023

Uh oh!

sjentzsch commented Oct 3, 2023

Uh oh!

tarrow commented Oct 4, 2023

Uh oh!

stale Bot commented Oct 18, 2023

Uh oh!

sjentzsch commented Oct 18, 2023

Uh oh!

tarrow commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

yxxhero commented Jan 25, 2026

Code Review

Summary

Code Changes

Concerns

Recommendation

Uh oh!

yxxhero commented Jan 25, 2026

Uh oh!

yxxhero commented Jan 25, 2026

Uh oh!

tarrow commented Jan 26, 2026

Uh oh!

yxxhero commented Jan 26, 2026

Uh oh!

tarrow commented Jan 28, 2026

Uh oh!

tarrow commented Feb 6, 2026

Uh oh!

yxxhero commented Feb 12, 2026

Uh oh!

yxxhero left a comment

Choose a reason for hiding this comment

Review Summary

Concerns

1. Breaking Change for preapply hooks ⚠️

2. Test Coverage

3. Rebase Needed

Minor Nit

Uh oh!

tarrow commented Jul 25, 2023 •

edited

Loading

mumoshu Aug 16, 2023 •

edited

Loading