-
Notifications
You must be signed in to change notification settings - Fork 583
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
ITL: add --extra-opts to plugins supporting it #8010
ITL: add --extra-opts to plugins supporting it #8010
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.
Hello @netphantm and thank you for contributing!
While updating the documentation you have indented the table columns of some new rows not as the others and re-indented the table columns of some existing rows not as the others and needlessly.
Please fix that.
Best,
AK
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.
-
git reset --soft HEAD~3
-
git commit --amend --no-edit
-
git push -f
6d92c32
to
77d2c58
Compare
I wanted to change also line 758, to escape the asterisk, since it changes the way it's rendered by vim w/ syntax highlighting from there onward (it makes all text italic), but changed it back to how it was initially.. |
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.
The ordering where you added the new option in the documentation is a bit inconsistent, see the individual comments.
Personal opinion (i.e. you don't have to change this, but in case you agree, feel free to do so): The existing options don't seem to follow a particular order in general, most likely it's based of the order in the usage output in the corresponding plugin. --extra-opts
seems to be some kind of "feature of last resort", i.e. you use it if you find no other way, therefore maybe it would make sense to always put it last?
@Al2Klimov Looks like this has been addressed, do you mind updating your review? |
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.
Btw. no need for rebasing or squashing – we'll take care.
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bruno Lingner.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bruno Lingner.
|
3 similar comments
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bruno Lingner.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bruno Lingner.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bruno Lingner.
|
@Al2Klimov I changed the email address, what do I have to do now to get the checks to be successful now? |
You have to change it in all commits here. |
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.
Btw. no need for rebasing or squashing – we'll take care.
The CLA bot seems to disagree with me. Please
- Checkout the branch
git reset --soft d5d89b7f398a0c8700fe36a64602a3e1bf3f52e0
- Commit all the stuff
I'll try to resolve the remaining open points myself as there was a customer request for this (in particular for Though when resolving the conflicts, I got confused about the icinga2/itl/command-plugins.conf Lines 363 to 367 in 866db3b
@yhabteab Looks like this was added based on your review. I don't see why this should be necessary though, like shouldn't arguments only be passed if the argument is set anyways? ref/IP/57417 |
Yes, I was thinking of a situation where a user might assign a value such as |
aaa518e
to
f9e117b
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Bruno Lingner.
|
I force pushed to squash the commits and rebased to resolve the conflicts. |
f9e117b
to
daf36ae
Compare
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @netphantm
|
And another force push to switch to the author's other e-mail address (there were commits with two different addresses on the branch, I changed it so that the one that's already in the AUTHORS file is used). The PR will be accepted without the CLA due to the trivial nature of the change (approved by @bobapple, see internal ticket ref/IP/57417). |
@cla-bot 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.
- The extra opts parameter is listed last in the documentation for every check except curl /
check_curl
, where it is first. - nscp /
check_nt
also supports--extra-opts
, https://www.monitoring-plugins.org/doc/man/check_nt.html. - smart /
check_ide_smart
also supports--extra-opts
, https://www.monitoring-plugins.org/doc/man/check_ide_smart.html.
Other plugins list --extra-opts last as it's often some kind of feature of last resort as it provides an option that can't be set in another way. For consistency, this also moves it to the end for the curl check command.
That's already added by the PR, am I missing something? https://github.com/netphantm/icinga2/blob/daf36ae362437d84ebf433045e51f655cce50087/doc/10-icinga-template-library.md#L1364 |
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.
Compared the documentation list against the CheckCommands and they seem to fit. Furthermore, I think we have not forgotten any check. Thanks everyone!
thanks guys, sorry I left this unfinished! I had a lot to deal with lately and also just simply forgot about it.. |
No worries! Thank you for starting it and already doing a lot of the work! |
Adding --extra-opts to nagios plugins
fixes #7630
fixes #10001