-
-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add preapply hook #79
Conversation
@Sajfer Can you add some unittest? Thanks very much. |
I am working on unittests, but I have ran into a problem.
The hook output contains a random 6 letter string that changes every run. |
@Sajfer try to use regex? |
Is there a need to document the defined behavior when specifying below hook events? 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. |
a02d4d8
to
733eb60
Compare
The current implementation only allows one of |
3f922a5
to
25484df
Compare
pkg/event/bus.go
Outdated
@@ -69,6 +71,10 @@ func (bus *Bus) Trigger(evt string, evtErr error, context map[string]interface{} | |||
} | |||
} | |||
|
|||
if slices.Contains(hook.Events, "preapply") && evt == "presync" && hook.Executed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or could you clarify the context behind this guard a bit more? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onapply
andsync
. - We do not want to have a prevention for the hook to run twice if both
preapply
andpresync
is added.
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds good.
I will see if I can get some time to make the changes this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
1252f45
to
12bb46c
Compare
@mumoshu I pushed an update that removes the prevention of double execution now. |
@Sajfer I will review this PR together. |
pkg/app/app.go
Outdated
@@ -1352,6 +1352,14 @@ Do you really want to apply? | |||
if !interactive || interactive && r.askForConfirmation(confMsg) { | |||
r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) | |||
|
|||
for _, release := range st.Releases { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, go ahead, it was not intended.
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Anton Bretting <sajfer@gmail.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
}) | ||
}) | ||
|
||
t.Run("apply release with preapply hook", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
@mumoshu Ready? |
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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.