Skip to content

Add check_smart.pl plugin as smart-advanced command#8041

Merged
yhabteab merged 1 commit intoIcinga:masterfrom
Napsty:check_smart
Jan 29, 2026
Merged

Add check_smart.pl plugin as smart-advanced command#8041
yhabteab merged 1 commit intoIcinga:masterfrom
Napsty:check_smart

Conversation

@Napsty
Copy link
Copy Markdown
Contributor

@Napsty Napsty commented Jun 3, 2020

This PR adds the check_smart.pl monitoring plugin under the smart-advanced check command (smart is already used by the check_ide_smart plugin).

@Al2Klimov Al2Klimov requested a review from htriem June 29, 2020 11:42
@Al2Klimov Al2Klimov requested review from julianbrost and removed request for htriem October 29, 2020 16:18
Comment thread doc/10-icinga-template-library.md Outdated
Comment thread doc/10-icinga-template-library.md Outdated
Comment thread doc/10-icinga-template-library.md Outdated
Comment thread doc/10-icinga-template-library.md Outdated
Comment thread itl/plugins-contrib.d/smart-advanced.conf
Comment thread doc/10-icinga-template-library.md Outdated
@Al2Klimov Al2Klimov added this to the 2.13.0 milestone Nov 23, 2020
@Napsty
Copy link
Copy Markdown
Contributor Author

Napsty commented Feb 25, 2021

Hi guys, do you still need something from me? I'm not sure from the notes above.

@julianbrost
Copy link
Copy Markdown
Member

I think we mainly have to agree on notation here. Meanwhile, I could remember a similar case with different checks: postgres where you can either pass a hostname or address as -H or a Unix Domain Socket as -s. But this case is special as -H is already set by default, but I suggest doing the following:

Always put the device name in smart_device (regardless of whether it's a glob or not) and have a second variable smart_device_is_glob that devices whether -d $smart_device$ or -g $smart_device$ is passed to the plugin.

@Al2Klimov What do you think of this?

For reference, this is how it's done for postgres:

"-H" = {
value = "$postgres_host$"
set_if = {{ macro("$postgres_unixsocket$") == false }}
description = "hostname(s) to connect to; defaults to none (Unix socket)"
}

@Napsty
Copy link
Copy Markdown
Contributor Author

Napsty commented Apr 22, 2021

@julianbrost OK let's try the latest version, see my commit ca9fcf3. I'm using the same way from your example with from postgres check. Never used this macro way though, so you'd have to tell me if that was correctly used.
Documentation adjusted.
New parameter smart_ssd_lifetime added.

Copy link
Copy Markdown
Member

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

In addition to the comments below, please also squash your changes into a single commit.

Comment thread doc/10-icinga-template-library.md Outdated
Comment thread doc/10-icinga-template-library.md Outdated
Comment thread itl/plugins-contrib.d/smart-advanced.conf Outdated
@julianbrost julianbrost removed this from the 2.13.0 milestone Jun 17, 2021
@cla-bot cla-bot Bot added the cla/signed label Feb 21, 2022
@julianbrost julianbrost self-requested a review February 21, 2022 14:16
Copy link
Copy Markdown
Member

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

The new file also has to be added in itl/plugins-contrib.d/CMakeLists.txt so that it will actually be used.

Comment thread doc/10-icinga-template-library.md Outdated
Comment thread itl/plugins-contrib.d/smart-advanced.conf Outdated
Comment thread doc/10-icinga-template-library.md Outdated
@waja
Copy link
Copy Markdown
Contributor

waja commented Jul 18, 2022

This is now 1y pending ... still no improvement yet.

@Napsty
Copy link
Copy Markdown
Contributor Author

Napsty commented Jul 18, 2022

Yes, that's on me. I always need some time to re-think into that issue every time, but some disasters always come in between. Sorry about that.

@Napsty Napsty requested a review from julianbrost October 31, 2022 06:19
Comment thread doc/10-icinga-template-library.md Outdated
@Al2Klimov Al2Klimov requested a review from julianbrost May 2, 2023 09:08
@Al2Klimov Al2Klimov added the area/itl Template Library CheckCommands label May 27, 2024
Copy link
Copy Markdown
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

I see some arguments in the docs that aren't part of this PR and I'm not sure whether they are new or not. However, this PR is open for far too long now, and I think I would just merge it unles @julianbrost says otherwise, since you're the one actively changes requested here, which AFAICS are all addressed.

@yhabteab yhabteab added this to the 2.16.0 milestone Jan 29, 2026
@julianbrost julianbrost dismissed their stale review January 29, 2026 14:54

My review was addressed.

@julianbrost
Copy link
Copy Markdown
Member

Yes, looks like what I requested was addressed, but then this PR completely fell under the radar. Sorry about that.

@yhabteab
Copy link
Copy Markdown
Member

I've rebased and squashed the commits! Thanks for your contribution.

@yhabteab yhabteab merged commit 6204386 into Icinga:master Jan 29, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/itl Template Library CheckCommands cla/signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants