-
Notifications
You must be signed in to change notification settings - Fork 20
Implicitly creating a threshold object in getopts rather than check_threshold #22
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
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.
Sounds good. Thanks
|
Let me have a look at the travis tests, they seem unrelated to this PR. |
|
could you please rebase this PR, should be fixed with f1e3e73 |
…eated when warning and critical is passed in, so that order of execution between $mp->check_threshold and $mp->add_perfdata doesn't matter
3b43a3e to
0b62478
Compare
|
@sni done 👍 |
|
thanks |
| $self->set_thresholds( | ||
| warning => $self->opts->warning, | ||
| critical => $self->opts->critical, | ||
| ) if ( defined $self->opts->warning && defined $self->opts->critical ); |
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 what if only one of the threshold is set (warning or critical)? Maybe I'm mistaken but I though that's a valid/common use case (maybe you want to only warn, never go critical...)
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.
thats a valid point. It previously checked $self->opts while it now checks critical and warning. This should at least be an or or reverted to the opts check.
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.
Yeah sure, we'll switch it to an or
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.
Previously, the call to
check_thresholdwould set the threshold on the Monitoring::Plugin object, if thresholds are defined, this means that these 2 snippets have different behaviour:To us, this is unexpected. The first snippet, will have thresholds on the perfdata, whereas the second will not. This PR should change the behaviour so that
$mp->thresholdwill exist from as early as possible, so need to runcheck_thresholdbeforeadd_perfdatais no longer there.Open to alternatives if this introduces some behaviour we're not aware of