Fix interactive apply asks in no change situation #945

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

pkg/app/app.go

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

sjentzsch · 2023-11-04T11:43:10Z

ping @mumoshu

yxxhero · 2024-02-11T04:06:04Z

@tarrow please fix ci issue.

tarrow · 2024-02-16T09:52:03Z

@yxxhero Hi!

As I mentioned in #945 (comment) I left these failing deliberately to illustrate the changes this PR would make and to confirm that @mumoshu was happy with them

#522 (comment)
preapply is supposed to get triggered even on unmodified releases

Did the helmfile team decide that you're happy to change this behaviour?

If so let me know and I'll update the snapshot :)

yxxhero · 2024-03-12T00:16:49Z

@mumoshu WDYT?

outdooracorn · 2025-06-24T08:51:26Z

@mumoshu please will you confirm that you are happy with the changes @tarrow has made so we can move this PR forward?

I find it very frustrating having to babysit helmfile apply in order to confirm no changes several times. I'm also worried I will get complacent about hitting y and accidentally deploy a change I haven't reviewed properly. Here is a snippet of my workflow for helmfile apply that I have to go through every time before viewing any actual changes:

Adding repo jetstack https://charts.jetstack.io
"jetstack" has been added to your repositories

Comparing release=cert-manager, chart=jetstack/cert-manager

Do you really want to apply?
  Helmfile will apply all your changes, as shown above.

 [y/n]: y
Comparing release=prometheus-operator-crds, chart=prometheus-community/prometheus-operator-crds

Do you really want to apply?
  Helmfile will apply all your changes, as shown above.

 [y/n]: y
Comparing release=istio-base, chart=istio/base

Do you really want to apply?
  Helmfile will apply all your changes, as shown above.

 [y/n]: y
Adding repo argo-cd https://argoproj.github.io/argo-helm
"argo-cd" has been added to your repositories

Comparing release=argo-cd-base, chart=argo-cd/argo-cd

Do you really want to apply?
  Helmfile will apply all your changes, as shown above.

 [y/n]: y
Adding repo tailscale https://pkgs.tailscale.com/helmcharts
"tailscale" has been added to your repositories

Comparing release=tailscale, chart=tailscale/tailscale-operator

Do you really want to apply?
  Helmfile will apply all your changes, as shown above.

 [y/n]: y

Thanks

Copilot

Pull Request Overview

This PR fixes the behavior of helmfile apply when running in interactive mode with no changes to apply. The main issue was that pre-apply hooks were being skipped when there were no releases to update or delete, and the interactive confirmation prompt was shown even when no changes were needed.

Moves the early exit condition for no changes to occur before the interactive confirmation check
Ensures pre-apply hooks always run before asking for confirmation in interactive mode
Updates documentation to clarify when pre-apply hooks will and won't run

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pkg/app/app.go	Adds early exit condition before interactive check to prevent unnecessary confirmation prompts when no changes are needed
docs/index.md	Updates documentation to clarify that preapply hooks won't run if there are no changes for any releases

davorgreentube · 2026-01-09T14:22:14Z

What is the reason this PR was newer merged?

tarrow · 2026-01-21T16:21:47Z

What is the reason this PR was newer merged?

I'm not sure.

I've gone ahead and also included the update to a test snapshot that is necessary now these hooks no longer run (in a no diff release). If someone could approve the workflow that would be great :)

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 Fixes: helmfile#679 Signed-off-by: Thomas Arrow <thomas.arrow@wikimedia.de>

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

docs/index.md


 `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!

Clarify conditions under which preapply hooks are triggered to include that they will no longer fire if there is a no-op. Recommended by the maintainer's copilot request

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

yxxhero added the in progress label Nov 2, 2023

tarrow force-pushed the ta/fixInteractiveOnNoChange branch from 9bbad20 to d886a49 Compare February 13, 2024 10:27

tarrow force-pushed the ta/fixInteractiveOnNoChange branch 2 times, most recently from d802031 to ed9f73a Compare March 11, 2024 14:58

yxxhero requested a review from zhaque44 as a code owner August 23, 2025 11:08

yxxhero requested a review from Copilot August 23, 2025 11:09

Copilot AI reviewed Aug 23, 2025

View reviewed changes

yxxhero force-pushed the ta/fixInteractiveOnNoChange branch from 0d0193f to a36fce9 Compare January 21, 2026 05:21

tarrow force-pushed the ta/fixInteractiveOnNoChange branch from a36fce9 to e1ec1cf Compare January 21, 2026 16:17

yxxhero force-pushed the ta/fixInteractiveOnNoChange branch from e1ec1cf to beb15ea Compare January 22, 2026 08:32

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 added 2 commits January 26, 2026 21:54

Update preapply hook description for clarity

78c5c80

Clarify conditions under which preapply hooks are triggered to include that they will no longer fire if there is a no-op. Recommended by the maintainer's copilot request

add code markdown

fc85cda

Uh oh!

Fix interactive apply asks in no change situation #945

Are you sure you want to change the base?

Fix interactive apply asks in no change situation #945

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!

sjentzsch commented Nov 4, 2023

Uh oh!

yxxhero commented Feb 11, 2024

Uh oh!

tarrow commented Feb 16, 2024

Uh oh!

yxxhero commented Mar 12, 2024

Uh oh!

outdooracorn commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

davorgreentube commented Jan 9, 2026

Uh oh!

tarrow commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tarrow commented Jul 25, 2023 •

edited

Loading

mumoshu Aug 16, 2023 •

edited

Loading

outdooracorn commented Jun 24, 2025 •

edited

Loading

tarrow commented Jan 21, 2026 •

edited

Loading