check_ping: Add support for showing name (DNS) resolution in the plugin output (STDOUT). (#1284)

[Mark A. Ziesemer]

@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?)

Thanks!

-- 
Reply to this email on GitHub:
https://github.com/monitoring-plugins/monitoring-plugins/pull/1284#issuecomment-64939909
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://www.monitoring-plugins.org/archive/devel/attachments/20141128/aa675e33/attachment.html>