Fix preapply hooks are not called on no diff when run apply subcommand

[xiaomudk]

Fix: #518
Signed-off-by: xiaomudk xiaomudk@gmail.com

[xiaomudk]

In this case, is this the expected value? @mumoshu

[mumoshu]

Yeah probably. I think this is an example of what we call "Release ID" in helmfile code. The first part is the kubecontext and the second part is a K8s namespace, followed by the release name. // looks like there were no namespace specified. Try setting namespace: whatever in the release so that the release ID would become default/whatever/bar.

Note that making it appear like default/bar when you omitted the release namespace doesn't look right, because then you have no way to distinguish a release whose namespace is omitted vs another release whose kubecontext is omitted.

[xiaomudk]

There is also a preapply problem. In this case, the chart has been declared not to be installed.
case

releases:
- name: bar
  chart: stable/mychart2
- name: foo
  chart: stable/mychart1
  installed: false
  needs:
  - bar

before
preapply will not be executed, I think it is reasonable

processing file "helmfile.yaml" in directory "."
changing working directory to "/path/to"
first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode=<nil>
first-pass uses: &{default map[] map[]}
first-pass rendering output of "helmfile.yaml.part.0":
 0: 
 1: releases:
 2: - name: bar
 3:   chart: stable/mychart2
 4: - name: foo
 5:   chart: stable/mychart1
 6:   installed: false
 7:   needs:
 8:   - bar
 9: 

first-pass produced: &{default map[] map[]}
first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]}
vals:
map[]
defaultVals:[]
second-pass rendering result of "helmfile.yaml.part.0":
 0: 
 1: releases:
 2: - name: bar
 3:   chart: stable/mychart2
 4: - name: foo
 5:   chart: stable/mychart1
 6:   installed: false
 7:   needs:
 8:   - bar
 9: 

merged environment: &{default map[] map[]}
2 release(s) found in helmfile.yaml

changing working directory back to "/path/to"

now
preapply will be executed, which is dangerous

more reasonable
Detect a release that needs to be installed and execute preapply. Whether this release needs to be updated or not.

@mumoshu @yxxhero @itscaro WDYT?

[mumoshu]

In this PR, we opted to make preapply happen even for releases that have no diff, right?
Then I'd guess it would also be reasonable to trigger preapply also for releases with installed: false, as the setting is intended to mean that "it's going to be deleted", not "exclude from the targeted releases". In other words, the release is going to be uninstalled as a part of the apply operation, hence the preapply hook should also be triggered.

In addition to that- could you elaborate a bit more on how it can be "dangerous"? Depending on the use case you had in your mind, we could discuss how to address it in another way. (Like, exposing a specified envvar to the hook command so that it can know the operation Helmfile is going to conduct on the release, for example delete, install, or upgrade)

[xiaomudk]

environments:
  test01:
  test02:

releases:
- name: bar
  chart: stable/mychart2
- name: foo
  chart: stable/mychart1
  installed: {{ eq .Environment.Name "production" test01 }}
  hooks:
    - events: ["preapply"]
      showlogs: true
      command: "echo"
      args: ["Hello from preapply"]

In this case, helmfile -e test02 apply will not install foo, but will execute preapply in foo, which is very dangerous if preapply has modifications to the cluster environment

[mumoshu]

Even the prepare hook is dangerous if you made it to conduct a "dangerous" operation on a cluster. The original motivation to add preapply was to add another extension point that is superior to existing hooks. presync doesn't get triggered on unmodified releases. Still, we needed preapply for use cases where you need it to be triggered on unmodified releases.

[mumoshu]

My advice would be to design your preapply hook command not to break your cluster when it's called on unmodified releases. Same for prepare.
Note also that whatever hook command should be made "idempotent", so that it is safe for rerunning. Helmfile doesn't guarantee that e.g. preapply is called only once for your release. For example, if helm-upgdate command failed after a preapply hook, you'd need to rerun the whole helmfile command and it would result in rerunning your preapply hook. If you've designed your preapply to do any dangerous operation, it would fail for the second time. That's an issue in your preapply hook, not Helmfile's preapply design.

[yxxhero]

@xiaomudk @mumoshu when a release will not be installed, should we auto-disable the hooks?

[mumoshu]

I think we'd better NOT disable hooks in that case. preapply is supposed to get triggered even on unmodified releases. It should work the same for releases with installed: false. installed: false declares the release to be deleted, unlike when you filtered out the release by using a label selector so that it is ever installed.
Indeed, we shouldn't call hooks for releases that are filtered out by using a label selector. But I think that's already implemented.

[xiaomudk]

I very much agree that hook command should be made "idempotent", I think it would be better to state this in the docs, I'll do it.

[yxxhero]

@xiaomudk adding more docs about pre-apply will be better.

[yxxhero]

@mumoshu WDYT?

[mumoshu]

@yxxhero Thanks for the reminder! Aren't we waiting for @xiaomudk to update the docs, or should I?

[xiaomudk]

@mumoshu @yxxhero Sorry, I will update the docs today

[yxxhero]

@mumoshu @itscaro WDYT?

[itscaro]

@yxxhero I need to check the behaviours in case of installed: false. I will get back to you ASAP.

[yxxhero]

@xiaomudk DCO

[itscaro]

Something is not consistent here:

When the chart is not installed and installed: false. No hook is called but preapply is, IMO, I not attend this. If preapply is called, prepare is attended to.

When the chart is installed, and we change installed: true to installed: false, prepare hook is not called, is it normal? since the document says

Hooks associated to prepare events are triggered after each release in your helmfile is loaded from YAML, before execution. prepare hooks are triggered on the release as long as it is not excluded by the helmfile selector(e.g. helmfile -l key=value).

I think when no action should be done, no hook should be called at all. I have cases that I only use installed to include or exclude a release and I do not use condition in combination with installed for the sake of ease.

[yxxhero]

Something is not consistent here:

When the chart is not installed and installed: false. No hook is called but preapply is, IMO, I not attend this. If preapply is called, prepare is attended to.

When the chart is installed, and we change installed: true to installed: false, prepare hook is not called, is it normal? since the document says

Hooks associated to prepare events are triggered after each release in your helmfile is loaded from YAML, before execution. prepare hooks are triggered on the release as long as it is not excluded by the helmfile selector(e.g. helmfile -l key=value).

I think when no action should be done, no hook should be called at all. I have cases that I only use installed to include or exclude a release and I do not use condition in combination with installed for the sake of ease.

see: #522 (comment)

@mumoshu

[itscaro]

@yxxhero I read the comment of mumoshu, I am partly OK. in the case, installed: false is not use to delete the chart but to not install the release, I feel something wrong to run hooks. Btw may be we should improve the documents on hooks to add some sequence diagrams, wdyt?

[yxxhero]

@yxxhero I read the comment of mumoshu, I am partly OK. in the case, installed: false is not use to delete the chart but to not install the release, I feel something wrong to run hooks. Btw may be we should improve the documents on hooks to add some sequence diagrams, wdyt?

good ieda.

[yxxhero]

@mumoshu So would we merge this PR?

[mumoshu]

installed: false is not use to delete the chart but to not install the release, I feel something wrong to run hooks

@itscaro I think I have similar sentiment as yours, but the behavior should be good in terms of consistency if we think carefully- it triggers the preapply hook regardless of whether there are any changes to be synced or not. Helmfile does diff between the missing(already uninstalled, not yet installed) release vs the desired state of the release installed: false. It syncs nothing due to there's no diff between the desired and the current states. But it does trigger the preapply hook because that's when preapply hook is supposed to be called.
I'm afraid my comment turned out to be a repetition of the previous one, but anyway, I hope it clarifies things a bit!

And yeah, any documentation enhancement should be welcomed if there's anything unclear.

[mumoshu]

I'm going to merge this anyway- but let's keep discussing! We should be able to freely change it to not get triggered for the installed: false + missing release case if we ended up having a consensus that's the desired behavior until this new preapply hook becomes popular.