Bug 433547 - Review Request: nsca - nagios passive check daemon
Review Request: nsca - nagios passive check daemon
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Xavier Bachelot
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-19 17:40 EST by Wart
Modified: 2008-04-03 17:54 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-04-03 17:54:44 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
xavier: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Wart 2008-02-19 17:40:26 EST
Spec URL: http://www.kobold.org/~wart/fedora/nagios-nsca.spec
SRPM URL: http://www.kobold.org/~wart/fedora/nagios-nsca-2.7.2-3.src.rpm
Description: 
NSCA is a daemon that lives on the nagios server and receives passive check results over the network.  It supports encrypted communications between the nsca client and nsca server.
Comment 1 Xavier Bachelot 2008-02-20 06:37:56 EST
fwiw, I was also working on nsca lately. I uploaded my package and spec if you
want to take a look (this is still a work in progress) :
http://washington.kelkoo.net/fedora/SPECS/nsca.spec
http://washington.kelkoo.net/fedora/SRPMS/nsca-2.7.2-2.fc8.src.rpm

I did not had time to look closely at your package but I will as soon as I have
a bit of time.

Regards,
Xavier
Comment 2 Wart 2008-02-21 15:26:30 EST
Hi Xavier,

It looks like we started from the same DAG rpm.  :)  Don't forget that the
/etc/nagios/*.cfg files need to be mode 0600 and readable only by root because
they contain encryption passwords.

Since you've also worked on the same package, would you like to review and/or
comaintain this with me?
Comment 3 Xavier Bachelot 2008-02-22 04:48:10 EST
(In reply to comment #2)
> Hi Xavier,
> 
Hi Wart,

> It looks like we started from the same DAG rpm.  :)  Don't forget that the
> /etc/nagios/*.cfg files need to be mode 0600 and readable only by root because
> they contain encryption passwords.
> 
> Since you've also worked on the same package, would you like to review and/or
> comaintain this with me?

Sure, that's the plan, but I'm short on time at the moment so I just pointed you
at my unfinished work so you may (or may not) cherry-pick from it. I'll get back
to it next week hopefully.

Regards,
Xavier
Comment 4 Xavier Bachelot 2008-02-28 12:42:32 EST
Hi Wart,

Here are my preliminary comments on your package :

Summary is not capitalized.

I like nsca-client much better than what I used. Is it really useful to suffix
the packages with nagios- though ? This would clear rpmlint output from 
nagios-nsca.i386: W: incoherent-subsys /etc/rc.d/init.d/nsca $prog
nagios-nsca.i386: W: incoherent-init-script-name nsca

nagios, nrpe and all are in the group Applications/System. You probably want to
use the same.

I don't have the BR: on libmcrypt-devel and my package build fine. Is it
necessary to keep it ?

I have some requires on /sbin/service and /sbin/chkconfig, taken from nrpe
packages, not sure this is needed, can't find anything in the packaging guidelines.
Requires(preun): /sbin/service, /sbin/chkconfig
Requires(post): /sbin/chkconfig, /sbin/service
Requires(postun): /sbin/service

NetSaint can probably be stripped out of the description.

Upstream ships an initscript (generated at build time actually) that only lacks
reload, it might not worth to replace it completely.

0600 on the conf files is good.

No doc in nsca-client, add %doc Changelog LEGAL README SECURITY

Nothing more for today, let me know what you think.

Regards,
Xavier
Comment 5 Xavier Bachelot 2008-03-03 06:41:28 EST
2 enhancements to my earlier comments: 
 
> I don't have the BR: on libmcrypt-devel and my package build fine. Is it
> necessary to keep it ?
> 
Indeed, this is needed... Sorry, temporary brain failure.

> I have some requires on /sbin/service and /sbin/chkconfig, taken from nrpe
> packages, not sure this is needed, can't find anything in the packaging
guidelines.
> Requires(preun): /sbin/service, /sbin/chkconfig
> Requires(post): /sbin/chkconfig, /sbin/service
> Requires(postun): /sbin/service
>
Yes, this is needed, here's the reference : 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e

I'll post an updated spec later.
Comment 6 Wart 2008-03-09 21:43:33 EDT
Here's another iteration based on the package that you mailed me (thanks!).  I
think we should keep 'NetSaint' in the description because we're not disabling
any netsaint functionality.  I also merged the sed script that was modifying the
init script into the existing init script patch.  It seemed silly to use both a
patch and a sed script for the same file:

http://www.kobold.org/~wart/fedora/nsca.spec
http://www.kobold.org/~wart/fedora/nsca-2.7.2-4.fc8.src.rpm

I haven't tried the new init script myself; I'll take your word that it works as
expected.
Comment 7 Xavier Bachelot 2008-03-10 14:39:53 EDT
(In reply to comment #6)
> Here's another iteration based on the package that you mailed me (thanks!).  I
> think we should keep 'NetSaint' in the description because we're not disabling
> any netsaint functionality. 

Ok

>  I also merged the sed script that was modifying the
> init script into the existing init script patch.  It seemed silly to use both a
> patch and a sed script for the same file:
> 
Agreed.

> http://www.kobold.org/~wart/fedora/nsca.spec
> http://www.kobold.org/~wart/fedora/nsca-2.7.2-4.fc8.src.rpm
> 
> I haven't tried the new init script myself; I'll take your word that it works as
> expected.

I need to diff the old one and the new one to make sure it's ok.

I see you actually used my spec as the new base, I hope this won't disqualify me
for the formal review. I'll put it in the next comment. Also this is my first
formal review, so I'll ask my sponsor to take a look.
Comment 8 Xavier Bachelot 2008-03-10 14:45:43 EDT
+ source files match upstream:
33a98e7975f633a9489d7a8938ed6131

+ package meets naming and versioning guidelines.

+ specfile is properly named, is cleanly written and uses macros consistently.

+ dist tag is present.

+ build root is correct.
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

+ license field matches the actual license.

+ license is open source-compatible. License text not included upstream.

+ latest version is being packaged.

+ BuildRequires are proper.

+ compiler flags are appropriate.

+ package builds in mock ( ).

+ package installs properly

+ debuginfo package looks complete.

+ rpmlint is not silent, but the 2 warnings are expected :
nsca.i386: E: non-readable /etc/nagios/nsca.cfg 0600
nsca-client.i386: E: non-readable /etc/nagios/send_nsca.cfg 0600

+ final provides and requires are sane:
Provides: config(nsca) = 2.7.2-4.el5
Requires(interp): /bin/sh /bin/sh /bin/sh
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires(post): /bin/sh /sbin/chkconfig
Requires(preun): /bin/sh /sbin/chkconfig /sbin/service
Requires(postun): /bin/sh /sbin/service
Requires: /bin/sh config(nsca) = 2.7.2-4.el5 libc.so.6 libc.so.6(GLIBC_2.0)
libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4) libc.so.6(GLIB
C_2.4) libmcrypt.so.4 libnsl.so.1 nagios rtld(GNU_HASH)

Provides: config(nsca-client) = 2.7.2-4.el5
Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
Requires: config(nsca-client) = 2.7.2-4.el5 libc.so.6 libc.so.6(GLIBC_2.0)
libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4)
libc.so.6(GLIBC_2.4) libmcrypt.so.4 libnsl.so.1 rtld(GNU_HASH)

+ no shared libraries are added to the regular linker search paths.
- owns the directories it creates.
nsca-client should own /etc/nagios because it doesn't require nagios.
nsca doesn't because it requires nagios.

+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ scriptlets follow guidelines.
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ not a GUI app.

just add %dir %{_sysconfdir}/nagios for the client and it should be good to go.

additional note : nsca-2.7.2-initreload.patch might need to be renamed now it
does more than just add reload to the init script.
Comment 9 Xavier Bachelot 2008-03-11 13:02:27 EDT
I went ahead with the 2 changes above and fixed the initscript at the same time
(broken reload, missing condrestart and both of them missing from the usage
message).

http://washington.kelkoo.net/fedora/SPECS/nsca.spec
http://washington.kelkoo.net/fedora/SRPMS/nsca-2.7.2-5.fc8.src.rpm
Comment 10 Shawn Starr 2008-03-17 18:52:32 EDT
You depending on the nagios package for both the client and server. Only
server needs nagios itself installed. 

What about the nagios user? Since your compiling NSCA requiring the nagios
using being present you'll need the user. This package should attempt to
create the user and otherwise silently ignore if it exists already.
Comment 11 Xavier Bachelot 2008-03-18 04:53:15 EDT
(In reply to comment #10)
> You depending on the nagios package for both the client and server. Only
> server needs nagios itself installed. 
>
Only nsca (the daemon) Requires: nagios. nsca-client doesn't.
 
> What about the nagios user? Since your compiling NSCA requiring the nagios
> using being present you'll need the user. This package should attempt to
> create the user and otherwise silently ignore if it exists already.

The nagios user is not required at compile time, the --with-nsca-user and
--with-nsca-grp option are only used to generate the default config. The nagios
user will only be needed at run time and then nsca Requires: nagios so it will
be already there.
Comment 12 Jason Tibbitts 2008-03-18 17:56:16 EDT
Xavier asked me to have a quick look.  Sorry I couldn't get to it yesterday.

A few meta-issues first:

Xavier, if you indeed are reviewing this, you should assign the ticket to
yourself and set the fedora-review flag to '?'.

The summary of the ticket should match the submitted package name.  Was it de

I don't have any particular issues with the reviewer providing fixed packages,
but  Michael does need to respond to the stuff in comments 8 and 9 before this
can move anywhere.

I am unfamiliar with nagios and related software, so I can really only look at
the basic packaging, which looks fine.  I'm looking at the package linked in
comment 9.  The scriptlets aren't exactly the same as the recommended ones, but
the differences are inconsequential.  Do note, however, that the "|| :" bit only
has an effect as the last executed command in a scriptlet; its sole purpose is
to prevent the scriptlet from returning a nonzero exit code.

Also note that I tend to just drop the bits of my review template that don't
apply, such as debuginfo and compiler flag bits for a noarch package, and the
GUI app bit for a non-gui app.  They're just placeholders to remind me to check
things when they apply.

Finally, I see some source files that are obviously GPLv2+, and others (nsca.c,
send_nsca.c) which say GPLv2.  Someone should ping upstream and see if they
intend GPLv2 only for the final product.  Otherwise I don't think GPLv2+ is
accurate for the license tag.
Comment 13 Xavier Bachelot 2008-03-18 19:35:36 EDT
Thanks for your comments Jason.
I've already addressed the issues from comment 8 in the package in comment 9. So
the last remaining issues would be the preun and postun scriptlets and the
License: tag.
Michael, could you please look at the scriptlets[1] and contact upstream about
the license issue ?

FWIW, I'm interested in co-maintaining this package, at least for EPEL.

[1]
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e
Comment 14 Wart 2008-03-18 19:47:34 EDT
Sorry for the silence.  Xavier has pretty much been proactive in updating the
review package as needed.  If he'd like to become the primary maintainer and
have me as a comaintainer, I'd be more than happy to hand this review over to him.

In the meantime I'll ping upstream about the licensing issue.
Comment 15 Xavier Bachelot 2008-03-18 20:00:36 EDT
I believe I can't be the primary maintainer and do the review at the same time.
You submitted the review, so you should be the primary maintainer. Like I wrote,
I'm genuinely interested in having this in EPEL, that's why I have been so
active. Sorry to be pushy, I certainly don't want to look as making a take over
on the package. Actually, I think we did a pretty good complementary job :-) 
Comment 16 Xavier Bachelot 2008-04-01 11:10:45 EDT
Wart, still around ? Did upstream get back to you wrt license ? Also, do you
want me to fix the scriplets to be as in the guidelines ? I'd like to get this
package built and released asap.
Comment 17 Wart 2008-04-01 23:37:26 EDT
Upstream never responded to the licensing question, so I've changed the license
to GPLv2 to best match the sources.  I also updated the scriplets to use  || :
on the last command.

http://www.kobold.org/~wart/fedora/nsca-2.7.2-6.fc8.src.rpm
http://www.kobold.org/~wart/fedora/nsca.spec

I think we're almost there...
Comment 18 Xavier Bachelot 2008-04-02 04:47:16 EDT
looks good, APPROVED.

I'm willing to co-maintain this package for EPEL. I don't mind being
comaintainer for Fedora too if you wish so. My username is xavierb.

Cheers,
Xavier
Comment 19 Wart 2008-04-03 16:07:55 EDT
New Package CVS Request
=======================
Package Name: nsca
Short Description: Nagios passive check daemon
Owners: wart, xavierb
Branches: F-8 EL-4 EL-5
InitialCC: 
Cvsextras Commits:
Comment 20 Kevin Fenzi 2008-04-03 16:19:02 EDT
cvs done.
Comment 21 Wart 2008-04-03 17:54:44 EDT
Imported and built for Rawhide.  Builds for F-8 and EL-{4,5} are coming along soon.

Note You need to log in before you can comment on or make changes to this bug.