Fix interactive apply asks in no change situation #945

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

This commit moves the pre-apply hooks that are now always run since #522 outside of the check for interactivity or asking for confirmation.

It also moves

the early exit condition for the case when there are no releases to Delete or Update
the cleanup of released with no change to before the interactivity check

This does subtly change the behaviour of preapply hooks when apply is invoked in interactive mode. The hooks will always be run before confirmation is asked for. In the event there are no releases to delete or update the preapply hooks will still run. Given that each preapply hook command must be idempotent I don't believe this is an issue.

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

@@ -1418,20 +1418,31 @@ Do you really want to apply?
 	// 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

stale · 2023-11-02T07:16:59Z

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

This commit makes the apply logic exit early in the event there are no changes to releases. I believe this effectively reverts helmfile#522. Fixes: helmfile#679 Signed-off-by: Thomas Arrow <[email protected]>

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

@mumoshu WDYT?

tarrow requested review from mumoshu, itscaro, yxxhero and xiaomudk 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

stale bot added the wontfix This will not be worked on label Nov 2, 2023

yxxhero added in progress and removed wontfix This will not be worked on labels 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 from d886a49 to d802031 Compare March 6, 2024 16:03

Fix interactive apply asks in no change situation

ed9f73a

This commit makes the apply logic exit early in the event there are no changes to releases. I believe this effectively reverts helmfile#522. Fixes: helmfile#679 Signed-off-by: Thomas Arrow <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix interactive apply asks in no change situation #945

Fix interactive apply asks in no change situation #945

tarrow commented Jul 25, 2023

yxxhero commented Jul 26, 2023

tarrow commented Jul 26, 2023

yxxhero commented Jul 26, 2023

tarrow commented Jul 31, 2023

tarrow commented Aug 14, 2023

yxxhero commented Aug 14, 2023

mumoshu Aug 16, 2023 •

edited

Loading

tarrow Sep 4, 2023

stale bot commented Sep 3, 2023

stale bot commented Sep 19, 2023

sjentzsch commented Sep 19, 2023

stale bot commented Oct 3, 2023

sjentzsch commented Oct 3, 2023

tarrow commented Oct 4, 2023

stale bot commented Oct 18, 2023

sjentzsch commented Oct 18, 2023

stale bot commented Nov 2, 2023

sjentzsch commented Nov 4, 2023

yxxhero commented Feb 11, 2024

tarrow commented Feb 16, 2024

yxxhero commented Mar 12, 2024

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

yxxhero commented Jul 26, 2023

tarrow commented Jul 26, 2023

yxxhero commented Jul 26, 2023

tarrow commented Jul 31, 2023

tarrow commented Aug 14, 2023

yxxhero commented Aug 14, 2023

mumoshu Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

tarrow Sep 4, 2023

Choose a reason for hiding this comment

stale bot commented Sep 3, 2023

stale bot commented Sep 19, 2023

sjentzsch commented Sep 19, 2023

stale bot commented Oct 3, 2023

sjentzsch commented Oct 3, 2023

tarrow commented Oct 4, 2023

stale bot commented Oct 18, 2023

sjentzsch commented Oct 18, 2023

stale bot commented Nov 2, 2023

sjentzsch commented Nov 4, 2023

yxxhero commented Feb 11, 2024

tarrow commented Feb 16, 2024

yxxhero commented Mar 12, 2024

mumoshu Aug 16, 2023 •

edited

Loading