check_ping: Add support for showing name (DNS) resolution in the plugin output (STDOUT). (#1284)
Mark A. Ziesemer
notifications at github.com
Sat Nov 29 04:17:51 CET 2014
@sni / @waja - Could we please have some opportunity for review /
discussion here before immediately squashing this as closed? I would be
more than happy to provide additional / alternate revisions here, if/as
required for inclusion. I think we'd all hate to have an additional fork
here for something so simple.
> since you almost always have the hostname in your interface or
> notification already and we'd like to avoid duplicate information.
This is backwards of what I intended. As I noted in the original pull
request above, I am monitoring everything by its DNS name, not IP. This is
for the reasons above, and is what I believe to be a better monitoring
configuration. As such, yes, we'd already have the hostname - but the IP is
then left missing. This is why "ping" provides these details for this very
reason - pinging "myHost.com"? Great - but which host did we resolve to?
If the IP changed to something "invalid" which is what caused the failure,
this option would allow us to see that immediately - without any additional
digging required, but only if enabled with the run-time option. Again,
we're not adding anything "extra" here - just no longer always preventing
useful information that the "ping" output already provided us with from
making it to the output.
> it still makes the code more complicated and less maintable
16 additional lines of code, largely templated off of the existing code?
> and reduces the number of available options for useful future features
Are we talking available command-line flags here? Let's just assume 36
single-characters to begin with ([A-Za-z0-9]), not including '?' and other
non-disputed allowed flags. We're currently using 12, and this would make
13. If we'd like to avoid using a single-character flag for this, that's
fine - and we can just stick with the long "--show-resolution" option.
> Also the whitespace seemed ok already, according to our guidelines (see
> CODE file) there should be
leading tabs followed by spaces.
I completely agree. However, if you review the current version of the file,
the indentation is not consistent. In re-reviewing my changes, the first 3
whitespace-related edits I made are probably up for debate, and I should
have left them alone. However, throughout the rest of the file -
indentations were being made with spaces only - not the "leading tabs
followed by spaces" which are required by the CODE file, and a preference
that I agree with and support.
> that should be done seperatly.
This is exactly why I made the whitespace changes in a separate commit - but
given that I was in this file anyway, I was hoping that a separate branch
and pull request would not be required here.
What can we please do to compromise here? If I submit a new pull request
with only the "--show-resolution" flag and supporting changes - no
additional single-character flag and no whitespace changes, will you please
reconsider? (Or is there anything we can please work with here without even
requiring all of that?)
Reply to this email on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Devel