roboll / helmfile

Deploy Kubernetes Helm Charts
MIT License
4.05k stars 564 forks source link

feat: Hooks #295

Closed mumoshu closed 6 years ago

mumoshu commented 6 years ago

I imagine that you use helmfile in combination with various tools 😃

I'd like to investigate the possibility to enhance helmfile.yaml, so that you can also declaratively manage which tools are executed how, on which timing, and so on. And I'd like to achieve it without implementing so many features into the helmfile core.

214 is one of feature requests relevant to this feature. #214 overrides specific helmfile internal task to use the specified command. So that helmfile could run any interchangeable command instead of e.g. helm upgrade --install to install/upgrade a helm release.

Hooks, on the other hand, would allow you to prepend or append arbitrary logics or commands to be executed along with a specific helmfile task.

For example:

Consider it as something like draft tasks or helm2 plugin hooks, or helm3 lifecycle event handlers.

Please share your use-cases! Thanks.

elemental-lf commented 6 years ago

As mentioned in https://github.com/roboll/helmfile/issues/214 I'd like some kind of hooks. I'd probably align them more to the helm side of things (that is preHelmUpgrade, postHelmUpgrade, preChartLoad) instead of the command line actions as a command line action might be a composite of actions (like sync and apply for example) and they are more likely to change yet, so there'd be no preApplyRun. Some thoughts: A failed pre-hook or post-hook should terminate helmfile. A post-hook should be called with status information about the preceding action to be able to react on it. Other information (like chart, release, revision) also needs to be passed along of course. To be able to use the same script to implement multiple hooks the name of the hook should also be passed along. Environment variables are one option to pass the information to the external program but templating is the more natural one in our case and more versatile. The template would need to be escaped so that the rendering is deferred which is a bit of a hassle.

mumoshu commented 6 years ago

@elemental-lf Hey, thanks a lot for awesome feedbacks!

align them more to the helm side of things (that is preHelmUpgrade, postHelmUpgrade, preChartLoad) instead of the command line actions as a command line action might be a composite of actions (like sync and apply for example) and they are more likely to change

Makes a lot of senses.

A failed pre-hook or post-hook should terminate helmfile

Agreed

A post-hook should be called with status information about the preceding action to be able to react on it

What kind of status information would you like? I guess, perhaps the oneline error message, and the whole stdout/stderr logs, so that we can create e.g. a slack notification post hook?

Other information (like chart, release, revision) also needs to be passed along of course. To be able to use the same script to implement multiple hooks the name of the hook should also be passed along.

Agreed.

I imagine we would start with:

same script to implement multiple hooks

Sounds great!

I was firstly considering a configuration syntax like:

releases:
- name: myapp
  chart: mychart
  hooks:
    preChartLoad:
     - command: {{` ./chartify -e {{ .Environment.Name }} {{ .Chart }} `}}

But to naturally support multiple hooks with a single script, I'd change it to:

releases:
- name: myapp
  chart: mychart
  hooks:
    chartify:
      phases: ["preChartLoad"] # add as many phases here
      command: {{` ./chartify -e {{ .Environment.Name }} {{ .Chart }} `}}

Environment variables are one option to pass the information to the external program but templating is the more natural one in our case and more versatile.

Agreed.

The template would need to be escaped so that the rendering is deferred which is a bit of a hassle.

Yeah. I'm still struggling to keep it simple as possible overall, but my best one so far is https://github.com/roboll/helmfile/issues/297#issuecomment-419611007. WDYT?

elemental-lf commented 6 years ago

@mumoshu, hope, you're well!

What kind of status information would you like? I guess, perhaps the oneline error message, and the whole stdout/stderr logs, so that we can create e.g. a slack notification post hook?

I just thought about signalling success or failure. But your Slack use-case would indeed profit from some more information especially when using helmfile in an automated setting.

But to naturally support multiple hooks with a single script, I'd change it to:

releases:

  • name: myapp chart: mychart hooks: chartify: phases: ["preChartLoad"] # add as many phases here command: {{./chartify -e {{ .Environment.Name }} {{ .Chart }}}}

Looks good. Why the extra level (chartify), why not directly below hooks? I think phases would be somewhat of a misnomer as a phase describes a time interval and hooks are more like a specific point in time, that is at the start of phase chartLoad or at the end of phase chartLoad. But I didn't come up with a better word just yet.

Should there be global hooks that are always executed regardless of the release? Maybe even with some extra hooks like helmfileStart or helmfileEnd? But maybe I'm starting to overthink this ;)

mumoshu commented 6 years ago

@elemental-lf Thanks as always for your feedback!

Why the extra level (chartify), why not directly below hooks?

I wanted to name each hook command, so that the user can make sense of which hook is running. But that's not a strong opinion.

I think phases would be somewhat of a misnomer as a phase describes a time interval and hooks are more like a specific point in time, that is at the start of phase chartLoad or at the end of phase chartLoad

Aha! Your explanation makes a lot of sense.

Should there be global hooks that are always executed regardless of the release? Maybe even with some extra hooks like helmfileStart or helmfileEnd?

Sounds very useful!

But maybe I'm starting to overthink this ;)

Probably :) But I tend to overthink things, too!

Anyway, I believe it's still valuable to "overthink" at first, so that we have more options to choose the minimum viable feature to start!

Now, I came up with 3 alternatives. My favorite is the third one below.

releases:
- ...
  hooks:
    chartify:
      points: ["preChartLoad"] # add as many points in the release lifecycle here
      command: {{` ./chartify -e {{ .Environment.Name }} {{ .Chart }} `}}
releases:
- ...
  plugins:
    chartify:
      hooks: ["preChartLoad"] # register this plugin to as many hooks as you want
      command: {{` ./chartify -e {{ .Environment.Name }} {{ .Chart }} `}}
releases:
- ...
  hooks:
    preChartLoad: # register as many commands as you want to this hook
    - command: {{` ./chartify -e {{ .Environment.Name }} {{ .Chart }} `}}
elemental-lf commented 6 years ago

Thanks for your quick reply. My favourite would also be your third example. The list should be executed in order to allow a chaining of commands depending on each other.

witten commented 6 years ago

I agree that it would be super nice to be able to declare a hook once in a helmfile.yaml and then make it apply globally to all releases therein, either implicitly or explicitly. For the use case, see #324.

Additionally, a side note: I'm the maintainer of a project that's also a YAML-based declarative wrapper for running another project, and I also recently grappled with the subject of adding hook support. There may be some patterns from that to apply here. (Or not.)

mumoshu commented 6 years ago

Thanks for the comment!

make it apply globally to all releases therein, either implicitly or explicitly

Agreed. I wrote #325 about this.

mumoshu commented 6 years ago

I also recently grappled with the subject of adding hook support. There may be some patterns from that to apply here. (Or not.)

Awesome! I just read the hooks section and am already inspired by it.

So conceptually you have before_EVENT on_EVENT and after_EVENT for every phase of borgmatic. For backup, you only have before_backup and after_backup because on_backup is (conceptually) implemented by borgmatic itself.

For error, you only have on_error because before_error and after_error doesn't make much sense because the error event means that the error has already been observed. If the event was named(maybe non-sense, this is just an example for my study) handle_error, before_handle_error or after_handle_error might have made sense.

Am I following your idea correctly? Anyway, it will provide me additional foundation to better shape the plugin hooks. Thx!

witten commented 6 years ago

Yup, that's all correct. And one thing to know about this as related to Helmfile is that all of the borgmatic hooks today are at the "global" level. There aren't (yet) any hooks at the per-repository level. And you can think of the repositories list as comparable to Helmfile's releases list.. I know you're thinking about global Helmfile hooks in #325.