-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fixes #24944: Add pre generation hook #5690
Fixes #24944: Add pre generation hook #5690
Conversation
Commit modified |
255eed2
to
042151f
Compare
res <- | ||
RunHooks.syncRun(preHooks, HookEnvPairs.build(("RUDDER_GENERATION_DATETIME", generationTime.toString)), systemEnv) match { | ||
case _: HookReturnCode.Success => ().succeed | ||
case x : HookReturnCode.Error => Inconsistency(s"${x.msg}\n stdout: ${x.stdout}\n stderr: '${x.stderr}'").fail |
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.
Since it will interrupt the policy generation and got up to user, I think we need a simpler error message than that ("policy generation interrupted by pre-check hook xxxx, error message: xxxxx" or something like that.
And still get that error message in log for the ops who will do the migration and need detailled info.
|
||
This directory contains hooks executed before policy generation. | ||
|
||
Typically, these hooks are used to check the integrity of techniques. |
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.
You need to explain that these hooks behave differently than all the other ones and that the will PREVENT policy generation on error.
In other cases, an error just stop the hook pipeline
|
||
# Errors code on hooks are interpreted as follow: | ||
# - 0 : success, no log (apart if debug one) , continue to next hook | ||
# - 1-31 : error , error log in /var/log/rudder/webapp/, stop processing |
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.
this need to be adapted.
I think "stop processing" should be updated in all hook template to be more precise on what is stop (ie just processing of other hooks) ; and on that one, very loudly say that it will prevent policy generation (and so possibly block all policy generation in case of error in the script)
/* | ||
* Pre generation hooks | ||
*/ | ||
override def runPreHooks(generationTime: DateTime, systemEnv: HookEnvPairs): Box[Unit] = { | ||
(for { | ||
preHooks <- RunHooks.getHooksPure(HOOKS_D + "/policy-generation-pre", HOOKS_IGNORE_SUFFIXES) |
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.
Perhaps pre-start
? Just pre
doesn't seems very clear
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 really think the user doc and error message reporting must be reworked, else users won't understand why this hook behave differently.
PR rebased |
042151f
to
1ace429
Compare
Commit modified |
1ace429
to
b2e17b0
Compare
Commit modified |
b2e17b0
to
f350cde
Compare
Commit modified |
f350cde
to
4081185
Compare
Commit modified |
4081185
to
9d676c5
Compare
OK, merging this PR |
572ec78
into
Normation:master
https://issues.rudder.io/issues/24944