-
Notifications
You must be signed in to change notification settings - Fork 39
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
[WIP] Adding hook support #456
base: master
Are you sure you want to change the base?
Conversation
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's the use case for the hooks? It was not mentioned in the commit message.
lago/hooks.py
Outdated
@@ -0,0 +1,254 @@ | |||
# coding=utf-8 | |||
import functools | |||
import os |
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.
Do we care about import order, alphabetically or something?
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.
will fix.
hooks = Hooks(prefix.paths.hooks()) | ||
hooks.run_pre_hooks(cmd) | ||
result = func(*args, **kwargs) | ||
hooks.run_post_hooks(cmd) |
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.
The hooks run regardless of the result, and are not aware of the result. Perhaps worth passing it to them?
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.
- Post hook won't run if 'func' throws an exception, do we want to change this behavior ?
- When passing 'result' to the post hook I need to assume that it is a string, or can be evaluated to a meaningful string, I'm not sure that I can take this assumption.
try: | ||
with utils.DirLockWithTimeout(cmd_dir): | ||
self._run_hooks( | ||
sorted([path.join(hook_dir, hook) for hook in hooks]) |
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.
hooks are running alphabetically?
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 want to preserve the order in which the hooks were mentioned in the init file, but now I realize that I didn't prefix them with an index. will fix.
|
Some possible use-cases of VM hooks:
Some possible use-cases of Host hooks:
|
1. The hooks are defined in the init file and can be any type of script (shell, python...). 2. The hooks will be run on the host which runs Lago. 3. Hooks will be only supported for functions that were decorated with "hooks.with_hooks" (for now just start / stop) Signed-off-by: gbenhaim <[email protected]>
6f7c05a
to
ab667df
Compare
The hooks are defined in the init file and
can be any type of script (shell, python...).
The hooks will be run on the host which runs Lago.
Hooks will be only supported for functions that were
decorated with "hooks.with_hooks" (for now just start / stop)
Signed-off-by: gbenhaim [email protected]