Documentation vague on "update-windows" check plugin

[shiz0]

Regarding this section in the docs:
https://docs.icinga.com/icinga2/latest/doc/module/icinga2/toc#!/icinga2/latest/doc/module/icinga2/chapter/plugin-check-commands#windows-plugins-update-windows

What values are expected to be set for update_win_warn/crit/reboot and what are their effects?
Are these booleans or thesholds?
Also they seem to be optional, but are not described as.

[dnsmichi]

Maybe you can extract details from check_update.exe -h or by looking into the source code: https://github.com/Icinga/icinga2/blob/master/plugins/check_update.cpp until @Crunsher comes back from holiday :)

[Crunsher]

"If set" would imply boolean, no? I'm not sure how to make it clearer, if you have an idea I'm happy to merge a pull request ^.^

[Crunsher]

@gunnarbeutner noted that these should be marked as optional like the others, I'll look into that

[shiz0]

Thank you @dnsmichi, "check_update.exe -h" indeed shed some light on how it works.
@Crunsher: Yes, you are right that the phrasing implies a boolean behaviour, but I wasn't fully convinced, probably because it is quite different than the bahaviour of other plugins where you set the thresholds etc.
Maybe it would be helpful to adopt some of the output of "check_update.exe -h" to the documenation.
I will spend some thoughts on that and I'll be happy to contribute - as soon as I've found out how todo it :-)
So, let's see if I got it right:

• Running the check without any additional variables wil return critical if any updates are pending.
• The optional variables activate the switches (-w -c) of the plugin wich alter the output as already described in the docs.
• The switches on the check are enabled by assigning any arbitrary value to the optional variables.
• Threshold for any behaviour of the plugin is always "1".

It that correct so far?

[Crunsher]

This is mostly correct. But regarding

The switches on the check are enabled by assigning any arbitrary value to the optional variables.

set_if evaluates boolean (true and false) and numbers (1.0 and 0.0), with strings it logs a warning and evaluates to false.

[Crunsher]

After further investigation: This ticket was created #5186

[shiz0]

Nice, I will take this into account when thinking about the documentation on this topic.
But I assume set_if is used elsewhere, too, so it should probably get an independend section in the docs and be referenced in the "update-windows" documentation.

[Crunsher]

It is, ticket #5186 exist for that purpose. All the check_update config needs is a clarification on the arguments being A Optional and B (boolean) toggles. Later we can add some reference to the table explaining what's evaluated true and false.

[Crunsher]

Fixed with pr

[dnsmichi]

Some notes for future reviews: Ensure that all commits contain "refs #issueid" as outlined in CONTRIBUTING.md. Further always assign a milestone before merging :)

I've now cherry-picked the doc fixes to 2.6.4 and updated docs.icinga.org (and amended the second commit to ref here).

[shiz0]

Yes, I will try todo better next time. This was my first time contributing on GitHub. :-)

[dnsmichi]

No worries, that comment targets us developers. But good git commit messages in PRs help a lot :)