Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HubLot
Copy link
Contributor

@HubLot HubLot commented Jan 13, 2015

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.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.12%) when pulling 079528d on HubLot:macros_validtime into 343943b on naparuba:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.42%) when pulling 4896d22 on HubLot:macros_validtime into 343943b on naparuba:master.

@HubLot
Copy link
Contributor Author

HubLot commented Jan 21, 2015

After some discussion with @gst, I made few improvements to the code.

@gst
Copy link
Contributor

gst commented Jan 21, 2015

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:

  1. if the timeperiod name present in the macro definition is unknown then:
    raise ValueError("macro xxx references the timepriod yyy but this one is undefined (did you made a typo?)")

  2. if the provided timestamp is not valid (not a valid float) :
    raise ValueError("macro xxx provided timestamp value (yyy) isn't valid : the-error-catched")

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).

@naparuba
Copy link
Contributor

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.

@HubLot
Copy link
Contributor Author

HubLot commented Feb 11, 2015

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).
But for the output, should I leave it unknown or raise a ValueError? Your last sentence is unclear @naparuba

@Seb-Solon Seb-Solon modified the milestone: 2.6 () Mar 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants