Thanks for great module!
Found few issues in D7 dev version:
1.Shown wrong link to configuration page at Admin status report page (admin/settings/nagios instead of admin/config/system/nagios)
2. Missing configuration link on module list page (not a bug, but it will be good to have reference on this page)
3. Path to basename command is not always /usr/bin/basename. On my CentOS it is a /bin/basename. Better to use 'basename' without path prefix.
Attached patch with changes
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | Remove-path-to-Linux-command-1480536-5.patch | 404 bytes | greg.harvey |
Comments
Comment #1
gregglesbetter status.
This looks sane to me, though I don't know check_drupal enough to know if that change is right.
Comment #1.0
greggles_
Comment #2
greg.harveyTwo of these changes have been committed in other patches, the third one makes sense but I find myself in the same position as greggles. I don't know it well enough. Assigning to kbahey for an opinion.
Comment #3
jweowu commentedWe're discussing the removal of the explicit path to the basename executable:
My take would be that it's unlikely to cause a problem. basename is a POSIX command, and *should* be in the PATH in any compliant environment. Moreover there are several other standard commands being called in check_drupal without explicit paths (mktemp, sed, awk). Others do have paths (rm, cp, egrep -- which should possibly be grep -E). Some are variously used with and without an explicit path (echo). It's all a little inconsistent.
I would expect that if the script is working at present, it will continue to work without an explicit path for basename.
Alternatively, defer it to a new issue to audit the path assumptions throughout the script.
This may be of interest: http://unix.stackexchange.com/questions/39244/does-posix-guarantee-that-...
Comment #4
greg.harveyRight, makes sense. That's probably actually *desirable*, e.g. let's not assume it's installed at /usr/bin, it might not be. I'm happy with that, just hadn't really clocked what the issue was. I will just run it by colleagues here, to sanity check.
Comment #5
greg.harveyOK, sanity checked! Here's a re-rolled patch, but the change is so trivial on reflection I might just commit it! :-)
Comment #7
greg.harveyOK, committed this. Unfortunately I couldn't attribute it to ph0enix because he has no Git account on Drupal.org.