-
Notifications
You must be signed in to change notification settings - Fork 564
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
feat: prepare
and cleanup
release event hooks
#349
feat: prepare
and cleanup
release event hooks
#349
Conversation
0ab9eef
to
5856c64
Compare
README.md
Outdated
@@ -593,6 +593,61 @@ mysetting: | | |||
|
|||
The possibility is endless. Try importing values from your golang app, bash script, jsonnet, or anything! | |||
|
|||
## Hooks | |||
|
|||
Hook is an per-release extension point of helmfile that is composed of: |
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.
What about this formulation?
A helmfile hook is a per-release extension point that is composed of:
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.
Sounds great!
README.md
Outdated
- `args` | ||
|
||
helmfile triggers various `events` while it is running. | ||
One `events` are triggered, associated `hooks` are executed, by running the `command` with `args`. |
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.
s/One/Once/
chart: mychart | ||
# *snip* | ||
hooks: | ||
- events: ["prepare", "cleanup"] |
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.
Is there a technical reason you didn't go with this the structure proposed in #330?:
hooks:
preChartLoad: # register as many commands as you want to this hook
- command: {{` ./chartify -e {{ .Environment.Name }} {{ .Chart }} `}}
I see that you would want to register the same command for a specific hook, but that seems to be over generic (I believe it is more common to have differing calls on each event, than the same call on multiple events).
YAML anchors could come handy if you have the same command on multiple events.
The preChartLoad
schema is verifiable at load time.
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.
Good catch! Indeed my intention was to make it even more generic with (hopefully) comparable complexity to the third option in my proposal.
I believe it is more common to have differing calls on each event, than the same call on multiple events
I think so too. My intention here was to provide the most DRY way to support this use-case:
To be able to use the same script to implement multiple hooks the name of the hook should also be passed along.
See #295 (comment) for more context.
The preChartLoad schema is verifiable at load time.
Good point! I was thinking to add validation for unsupported events. That is, events: ["foo"]
results in an error like unsupported event to hook: foo
.
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.
And I wasn't sure if "eventsare the only possible hook target in helmfile. Googling around, the term
hook` seems to be used for NOT only for expressing extension points associated to events. For example, something like "action hook" can be considered:
hooks:
- actions: ["lint"]
command: ...
An event hook reacts on specific event and doesn't have ability to return data from the hook to helmfile.
An action hook, on the other hand, could be used for passing data from the hook to helmfile, in an output format helmfile expects for the specific action. In the above example, the lint
action hook can return a specifically formatted json to add lint results that are integrated into the original output of helmfile lint
.
Explicitly wording hook targets as events
prevents ambiguity and creates future possibility like this.
small nit: (title contains a typo: |
5856c64
to
f6a9576
Compare
README.md
Outdated
|
||
Run `helmfile --environment staging sync` and see it results in helmfile running `kustomize build foo-kustomize/overlays/staging > foo/templates/all.yaml`. | ||
|
||
Viola! You can mix helm releases that are backed by remote charts, local charts, and even kustomize overlays. |
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.
s/Viola/Voilà/
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.
Good catch! Thanks. Fixed in the latest commit.
prepare
and cleanup
release event hooksprepare
and cleanup
release event hooks
f6a9576
to
8886038
Compare
helmexec/runner.go
Outdated
|
||
// Execute a shell command | ||
func (shell ShellRunner) Execute(cmd string, args []string) ([]byte, error) { | ||
preparedCmd := exec.Command(cmd, args...) | ||
if shell.Dir != "" { | ||
preparedCmd.Dir = shell.Dir |
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.
What do you think about setting preparedCmd.Dir
unconditionally? It's default value is the empty string.
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 believe we shouldn't, because we use it to run helm commands and it assumes Dir unspecified today.
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.
What I meant is that the condition is not necessary:
if shell.Dir is empty, assign (it won't change the default of preparedCmd which is empty)
if shell.Dir is not empty, assign (as you you in the condition)
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.
Ah makes sense! I'll fix it asap. Thanks for your review!
state/hook.go
Outdated
allargs := []string{} | ||
allargs = append(allargs, command[1:]...) | ||
allargs = append(allargs, args...) | ||
bytes, err := state.runner.Execute(command[0], allargs) |
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.
command is a slice, but only the first one is executed. How does that work if the user specifies multiple commands ?
command: ["echo", "helm"]
args: ["hello", "world", "list"]
What do you think about these UIs?:
command: ["command", "arg1", "arg2"]
or
commands:
- cmd: "helm"
args: "list"
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.
Trailing items in the command array is considerd args. I made it so to make it consistent with k8s pod spec, hoping it is easy to remember along with it.
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.
OK, but the podSpec is meant to override CMD and ENTRYPOINTS of containers. Not sure the model applies. Your call :-)
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.
Good point! Maybe blindly trying to make it consistent with pod spec doesn't make sense. I'll just make it a singular value. Thanks a lot for your comments 👍
I believe this change could benefit from additional tests. |
8886038
to
5e528c0
Compare
@davidovich I've added tests and some tweaks. Would you mind confirming? Thanks! |
Will have a look tomorrow morning
Le mer. 19 sept. 2018 à 22:36, KUOKA Yusuke <notifications@github.com> a
écrit :
… @davidovich <https://github.com/davidovich> I've added tests and some
tweaks. Would you mind confirming? Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#349 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABX6cuEWk-7ps693ETdIOCIxAtz_FlNPks5ucv8tgaJpZM4Wvzqj>
.
|
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.
Only one comment on the readme
LGTM!
README.md
Outdated
# *snip* | ||
hooks: | ||
- events: ["prepare", "cleanup"] | ||
command: ["echo"] |
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.
Upon your latest changes, I believe this is not an array anymore, what do you think?
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.
Good catch! I'll fix it asap.
Resolves roboll#295 Resolves roboll#330 Resolves roboll#329 (Supports templating of only `releases[].hooks[].command` and `args` right now Resolves roboll#324
5e528c0
to
5c67644
Compare
@davidovich Thanks a lot for your review |
Resolves #295
Resolves #330
Resolves #329 (Supports templating of only
releases[].hooks[].command
andargs
right nowResolves #324