Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 9, 2020
Merged

Implicitly creating a threshold object in getopts rather than check_threshold #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions lib/Monitoring/Plugin.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,7 @@ sub check_threshold {

# in order of preference, get warning and critical from
# 1. explicit arguments to check_threshold
# 2. previously explicitly set threshold object
# 3. implicit options from Getopts object
# 2. previously explicitly set threshold object or implicit theshold object created by warning and critical
if ( exists $args{warning} || exists $args{critical} ) {
$self->set_thresholds(
warning => $args{warning},
Expand All @@ -142,12 +141,6 @@ sub check_threshold {
elsif ( defined $self->threshold ) {
# noop
}
elsif ( defined $self->opts ) {
$self->set_thresholds(
warning => $self->opts->warning,
critical => $self->opts->critical,
);
}
else {
return UNKNOWN;
}
Expand All @@ -163,6 +156,10 @@ sub add_arg {
sub getopts {
my $self = shift;
$self->opts->getopts(@_) if $self->_check_for_opts;
$self->set_thresholds(
warning => $self->opts->warning,
critical => $self->opts->critical,
) if ( defined $self->opts->warning && defined $self->opts->critical );
Copy link
Member

@dermoth dermoth Sep 10, 2020

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

Copy link
Contributor

@sni sni Sep 14, 2020

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.

Copy link
Contributor Author

@infraweavers infraweavers Sep 14, 2020

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

Copy link
Contributor Author

@infraweavers infraweavers Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dermoth @sni fix: #23

}

sub _check_for_opts {
Expand Down