-
Notifications
You must be signed in to change notification settings - Fork 334
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
Implementation of special on-demand macros (isvalidtime & nextvalidtime) #1452
base: master
Are you sure you want to change the base?
Conversation
After some discussion with @gst, I made few improvements to the code. |
but wait : I've read the doc about these 2 "special" macros, and I'm still HIGHLY in favor of simply letting an error raise in case the provided timeperiod (or optional timestamp) value(s) are undefined or not valid... (instead of completely silencing the possible errors and returning some default value) This should be done at the config parsing level, at some stage.. can you check ? (I agree that's not necessarily easy to spot and figure out where exactly (and how)). anyway : what you can already do for this code fragment:
that's kind of defensive programming ; better to raise an explicit exception (with clear message of what's wrong) than to silence the missed/failed thing(s) and return some default value which will beat your sooner than latter (but you'll have difficulties to know why). |
I prefer a warning log thant a conf parsing that evaluate all (you can set comand line at run, so it's useless). A valuerror can be a problem, means that the command can't be launch? so unknown output for the command in fact. We did never break command for unknown macro or things like this, but why not with such argument macro after all. |
I agree with the need of a warning log at least so people can understand the problem (it tooks me time to figure out why these 2 macros didn't return a valid output). |
I noticed that 2 on-demand macros are not implemented :
They were always empty when I tried to use them in event handlers or command.
I suggest here an implementation of these 2 macros. As I'm not really familiar with shinken code, I tried to implement them in the best location (i.e in shinken/macroresolver.py).
The idea was to test them before any other on-demand macros (services or hosts).
Let me know what you think.