Add preapply hook by Sajfer · Pull Request #79 · helmfile/helmfile

Sajfer · 2022-04-30T13:05:34Z

As discussed in roboll/helmfile#1940 this PR adds a preapply hook that will run every time the release is applied, even if there is no changes to the release.

yxxhero · 2022-05-01T02:54:19Z

@Sajfer Can you add some unittest? Thanks very much.

Sajfer · 2022-05-01T08:29:07Z

I am working on unittests, but I have ran into a problem.

--- FAIL: TestApply_2 (0.04s)
    --- FAIL: TestApply_2/apply_release_with_preapply_hook (0.00s)
        /home/sajfer/code/helmfile/pkg/app/app_apply_test.go:1286: unexpected log: want (-), got (+):   strings.Join({
              	... // 1131 identical bytes
              	"ing preapply hook for foo:\nhook[echo]: stateFilePath=helmfile.ya",
              	"ml, basePath=.\n\nhook[echo]: triggered by event \"preapply\"\n\necho:",
            - 	"DKdXX",
            + 	"pFGpk",
              	"> foo\nhook[echo]: foo\n\n\n\nhook[preapply] logs | foo\nhook[preapply",
              	"] logs | \nprocessing 1 groups of releases in this order:\nGROUP R",
              	... // 299 identical bytes
              }, "")

The hook output contains a random 6 letter string that changes every run.

yxxhero · 2022-05-01T08:56:32Z

I am working on unittests, but I have ran into a problem.

--- FAIL: TestApply_2 (0.04s)
    --- FAIL: TestApply_2/apply_release_with_preapply_hook (0.00s)
        /home/sajfer/code/helmfile/pkg/app/app_apply_test.go:1286: unexpected log: want (-), got (+):   strings.Join({
              	... // 1131 identical bytes
              	"ing preapply hook for foo:\nhook[echo]: stateFilePath=helmfile.ya",
              	"ml, basePath=.\n\nhook[echo]: triggered by event \"preapply\"\n\necho:",
            - 	"DKdXX",
            + 	"pFGpk",
              	"> foo\nhook[echo]: foo\n\n\n\nhook[preapply] logs | foo\nhook[preapply",
              	"] logs | \nprocessing 1 groups of releases in this order:\nGROUP R",
              	... // 299 identical bytes
              }, "")

The hook output contains a random 6 letter string that changes every run.

@Sajfer try to use regex?

gakesson · 2022-05-01T09:02:22Z

Is there a need to document the defined behavior when specifying below hook events?
events: ["presync", "preapply"]

I would expect/want the hook to only be executed once for the release in question in above scenario but I think it is good if we establish and document a well-known behavior.

Sajfer · 2022-05-04T20:18:31Z

The current implementation only allows one of preapply and presync to run.
I have added some tests that checks that as well.

Sajfer · 2022-05-09T07:35:45Z

@itscaro @mumoshu Any chanse for a review? :)

Sajfer · 2022-05-18T09:59:17Z

@itscaro @mumoshu can you review?

mumoshu · 2022-06-05T04:31:40Z

 			}
 		}

+		if slices.Contains(hook.Events, "preapply") && evt == "presync" && hook.Executed {


I think this special handling is unnecessary- when one included a hook for both events preapply and presync, almost probably it's what they wanted, for e.g. notifying the timing helmfile triggered respective events. There shouldn't be a hard-coded logic to block such usage.

Or could you clarify the context behind this guard a bit more? 🙏

@yxxhero @itscaro Do you have any insights on this? I'm wondering if I'm missing any practical use-cases of this feature that might make this special handling necessary

@mumoshu I added it as a response to @gakesson, it felt reasonable, but I can remove it if not wanted.

The reasoning was that we can add both preapply and presync to make sure that the hook is run both if apply or sync is used, but that it only should run once. Since the presync hook is called when running apply as well, if there is any changes that will be synced to the cluster.

@Sajfer @mumoshu it was mostly that to me it is not intuitive that if I run "helmfile apply" and have events as ["presync", "preapply"] that the hook would be executed twice (in case sync is invoked for the release), but perhaps this type of special handling is not consistent how events and hooks work today.
But I'm fine either way, as long as we have a well-defined and documented behavior.

@mumoshu The use-case I was thinking of was that the hook execution would behave same (=execute once always) irrespective of our users use sync or apply (we deliver our helmfile to other parts of company org). In that case we would specify ["preapply", "presync"] to cover both rollout commands, but obviously this can instead be solved by simply instructing users to only use apply and never sync if we use the preapply event. And at least for us it is not a problem to execute the hook twice, only a bit redundant.
But for consistency I think it is fine to not have this special handling. Sorry for the confusion on this discussion. :)

If we only have the preapply hook set and run helmfile sync then that hook would not run right?

@Sajfer Ah, good point! And that's how it's implemented in this PR now, right? I'd say the new hook should be triggered also on helmfile sync.

It's unfortunately that presync and preapply doesn't correspond 1:1 to helmfile sync and helmfile apply respectively. But still, to better reflect how it's supposed to work:

preapply hooks will trigger everytime a release is applied, indifferent if there is changes to the release or not.

we'd better make it triggered also on helmfile-sync.

Correct, the current implementation only runs on apply and not on sync.

Just to be clear so there is no more changes after:

we want the preapply hook to run both on apply and sync.

We do not want to have a prevention for the hook to run twice if both preapply and presync is added.

Is this correct?

@Sajfer Yes, you're absolutely correct. I think the combination of those two changes makes the new preapplly hook more consistent with other hooks(Each hook is run only once per release on a lifecycle event of each release, regardless of which helmfile sub-command you're using). Does it make sense to you?

Yes, that sounds good.
I will see if I can get some time to make the changes this week.

mumoshu

@Sajfer Sorry for the delayed review! Just one comment regarding the guard for preventing double-running a hook in case it was targeted both presync and preapply. WDYT?

Sajfer · 2022-06-11T11:55:55Z

@mumoshu I pushed an update that removes the prevention of double execution now.

yxxhero · 2022-06-11T12:07:16Z

@Sajfer I will review this PR together.

mumoshu · 2022-09-18T09:52:17Z

 	if !interactive || interactive && r.askForConfirmation(confMsg) {
 		r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...)

+		for _, release := range st.Releases {


@Sajfer Thanks a lot for the fix! Juts to be extra sure, you're triggering preapply events also for unprocessed(=not-deleted and not-updated releases without any changes to be applied) releases. Is it intentional? If not, I'm prepared for fixing it in a way I've summarized in my previous comment.

Sure, go ahead, it was not intended.

Signed-off-by: Anton Bretting <[email protected]>

Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu · 2022-09-19T01:40:34Z

+		})
+	})
+
+	t.Run("apply release with preapply hook", func(t *testing.T) {


@Sajfer I've never realized that you were able to use the same test title multiple times like this (you have three of them in this file) and the framework still differentiates those by appending suffixes like #2 and #3. Great finding!

yxxhero · 2022-09-19T02:03:53Z

@mumoshu Ready?

Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu · 2022-09-19T02:37:45Z

@Sajfer I've added documentation about this feature in 385c3e8. It would be great if you could take a look and leave any comments if any!

mumoshu

I've added a few more test cases for verification and it turned out to be working well! This LGTM now.
Thanks a lot for your patience and awesome work @Sajfer!

Sajfer requested review from itscaro and mumoshu as code owners April 30, 2022 13:05

Sajfer mentioned this pull request Apr 30, 2022

presync hook not running with apply roboll/helmfile#1940

Open

Sajfer force-pushed the preapply branch from 9775b2f to d622a3e Compare April 30, 2022 13:08

Sajfer changed the title ~~Add preapply hook~~ Draft: Add preapply hook May 1, 2022

Sajfer marked this pull request as draft May 1, 2022 09:53

Sajfer force-pushed the preapply branch 4 times, most recently from a02d4d8 to 733eb60 Compare May 4, 2022 20:03

Sajfer marked this pull request as ready for review May 4, 2022 20:17

Sajfer changed the title ~~Draft: Add preapply hook~~ Add preapply hook May 5, 2022

Sajfer force-pushed the preapply branch from 5e0f5a4 to b06023d Compare May 10, 2022 12:20

Sajfer force-pushed the preapply branch from b06023d to 0381d74 Compare May 18, 2022 09:57

Sajfer force-pushed the preapply branch 2 times, most recently from 3f922a5 to 25484df Compare May 25, 2022 17:45

mumoshu reviewed Jun 5, 2022

View reviewed changes

Sajfer requested a review from yxxhero as a code owner June 11, 2022 11:52

Sajfer force-pushed the preapply branch 2 times, most recently from 1252f45 to 12bb46c Compare June 11, 2022 11:54

mumoshu reviewed Sep 18, 2022

View reviewed changes

Comment thread pkg/app/app.go Outdated

Sajfer force-pushed the preapply branch from 7d1b753 to 5ae34e5 Compare September 18, 2022 07:42

mumoshu reviewed Sep 18, 2022

View reviewed changes

Sajfer and others added 14 commits September 19, 2022 08:55

Add preapply hook

4e5987d

Signed-off-by: Anton Bretting <[email protected]>

Add unittests for preapply

1a3c11d

Signed-off-by: Anton Bretting <[email protected]>

Only run preapply or presync

db81f18

Signed-off-by: Anton Bretting <[email protected]>

Fix merge conflicts

32477e6

Signed-off-by: Anton Bretting <[email protected]>

Remove logic that limits execution of hooks to only once

c6e8afd

Signed-off-by: Anton Bretting <[email protected]>

Remove unnecessary if statement

eb2419a

Signed-off-by: Anton Bretting <[email protected]>

Uncomment code that was accidentally commented out

ca7942c

Signed-off-by: Anton Bretting <[email protected]>

Move preapply code to separate function

ea56061

Signed-off-by: Anton Bretting <[email protected]>

Add unittests for new preapply functions

fcdd852

Signed-off-by: Anton Bretting <[email protected]>

Fixes for updates from rebase

f0b76e9

Signed-off-by: Anton Bretting <[email protected]>

Fix lint errors

1e9cce7

Signed-off-by: Anton Bretting <[email protected]>

Update documentation

5b88006

Signed-off-by: Anton Bretting <[email protected]>

Updates based on review comments

3c0456c

Signed-off-by: Anton Bretting <[email protected]>

fixup! Updates based on review comments

5b1606d

Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu force-pushed the preapply branch from eefb691 to 5b1606d Compare September 18, 2022 23:55

mumoshu added 2 commits September 19, 2022 00:27

Write preapply notice to debug log instead of info

9e673ca

Signed-off-by: Yusuke Kuoka <[email protected]>

Defer implementing preapply hook start logs to another commit/pr

793050c

Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu reviewed Sep 19, 2022

View reviewed changes

mumoshu added 3 commits September 19, 2022 02:19

Add more testcases for hooks

dc40ccd

Signed-off-by: Yusuke Kuoka <[email protected]>

Add documentation about the new preapply hook

385c3e8

Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! Add more testcases for hooks

bb13ef6

Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu approved these changes Sep 19, 2022

View reviewed changes

yxxhero merged commit 24810a4 into helmfile:main Sep 19, 2022

Sajfer deleted the preapply branch September 19, 2022 07:52

github-actions Bot locked as resolved and limited conversation to collaborators Sep 21, 2022

Uh oh!

Uh oh!

Conversation

Sajfer commented Apr 30, 2022

Uh oh!

yxxhero commented May 1, 2022

Uh oh!

Sajfer commented May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yxxhero commented May 1, 2022

Uh oh!

gakesson commented May 1, 2022

Uh oh!

Sajfer commented May 4, 2022

Uh oh!

Sajfer commented May 9, 2022

Uh oh!

Sajfer commented May 18, 2022

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!

Sajfer Jun 6, 2022 • 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!

mumoshu left a comment

Choose a reason for hiding this comment

Uh oh!

Sajfer commented Jun 11, 2022

Uh oh!

yxxhero commented Jun 11, 2022

Uh oh!

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!

yxxhero commented Sep 19, 2022

Uh oh!

mumoshu commented Sep 19, 2022

Uh oh!

mumoshu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sajfer commented May 1, 2022 •

edited

Loading

Sajfer Jun 6, 2022 •

edited

Loading