Bug 743615 - Review Request: nagios-plugins-openmanage - Nagios plugin to monitor hardware health on Dell servers
Summary: Review Request: nagios-plugins-openmanage - Nagios plugin to monitor hardwar...
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-10-05 14:21 UTC by Trond H. Amundsen
Modified: 2012-04-14 18:00 UTC (History)
5 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2012-04-13 21:31:54 UTC
tomspur: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Trond H. Amundsen 2011-10-05 14:21:49 UTC
Spec URL: http://folk.uio.no/trondham/review/nagios-plugins-openmanage.spec
SRPM URL: http://folk.uio.no/trondham/review/nagios-plugins-openmanage-3.7.3-1.el6.src.rpm
Description:
check_openmanage is a plugin for Nagios which checks the hardware
health of Dell servers running OpenManage Server Administrator
(OMSA). The plugin can be used remotely with SNMP or locally with
NRPE, check_by_ssh or similar, whichever suits your needs and
particular taste. The plugin checks the health of the storage
subsystem, power supplies, memory modules, temperature probes etc.,
and gives an alert if any of the components are faulty or operate
outside normal parameters.

Comment 1 Terje Røsten 2011-10-05 16:34:23 UTC
# No binaries here, do not build a debuginfo package
%global debug_package %{nil}

Why is not use BuildArch: noarch?


URL:           http://folk.uio.no/trondham/software/%{plugin}.html
Source0:       http://folk.uio.no/trondham/software/files/%{plugin}-%{version}.tar.gz

I don't see the value in using the %{plugin} macro here.

BuildRequires: /usr/bin/pod2man

Well, simply using BuildRequires: perl should be safe and faster?

Requires:      perl(Config::Tiny)
Requires:      perl(Net::SNMP)
Requires:      perl(Crypt::Rijndael)

Please let rpm find these.

You will find this page useful: 

http://fedoraproject.org/wiki/Packaging:Perl

Provides:      nagios-plugins-check-openmanage = %{version}-%{release}
Obsoletes:     nagios-plugins-check-openmanage < 3.7.2-3

Please explain the need for these lines.

%{nagiospluginsdir}/*
%{_mandir}/man8/*.8*
%{_mandir}/man5/*.5*
%dir %{_sysconfdir}/nagios
%config(noreplace) %{_sysconfdir}/nagios/*

1)I like more explicit file list, however that's me.
2) I guess %dir %{_sysconfdir}/nagios is owned by nagios-plugins
and not needed in this package?

Would be nice if you could create a FAS account and do a koji scratch build.

Comment 2 Trond H. Amundsen 2011-10-05 16:59:22 UTC
(In reply to comment #1)
> # No binaries here, do not build a debuginfo package
> %global debug_package %{nil}
> 
> Why is not use BuildArch: noarch?

Because I use the %{_libdir} macro. As far as I can see, the placement for Nagios plugins in Fedora/EPEL is /usr/lib64/nagios/plugins and /usr/lib/nagios/plugins for 64bit and 32bit arches, respectively. If not for this, it would be a noarch package.

> URL:           http://folk.uio.no/trondham/software/%{plugin}.html
> Source0:      
> http://folk.uio.no/trondham/software/files/%{plugin}-%{version}.tar.gz
> 
> I don't see the value in using the %{plugin} macro here.

Mostly cosmetic reasons. I couldn't use %{name} :)

> BuildRequires: /usr/bin/pod2man
> 
> Well, simply using BuildRequires: perl should be safe and faster?

Maybe. If it's safer/better/preferred to use package names instead in BuildRequires I'll change it.

> Requires:      perl(Config::Tiny)
> Requires:      perl(Net::SNMP)
> Requires:      perl(Crypt::Rijndael)
> 
> Please let rpm find these.

No, rpmbuild doesn't find these. The first two aren't found because they're only invoked if the user requests a certain feature via options, i.e. there are no "use Foo::Bar" that rpmbuild would find. The last one is only needed if the user wishes to use SNMPv3 with AES. Neither the plugin nor Net::SNMP requires it.

> You will find this page useful: 
> 
> http://fedoraproject.org/wiki/Packaging:Perl

Thanks.

> Provides:      nagios-plugins-check-openmanage = %{version}-%{release}
> Obsoletes:     nagios-plugins-check-openmanage < 3.7.2-3
> 
> Please explain the need for these lines.

I have for a long time supplied RPM packages for download (not part of a repo). These lines are included to make the transition to Fedora/EPEL packages easier for existing users of the RPM packages.

> %{nagiospluginsdir}/*
> %{_mandir}/man8/*.8*
> %{_mandir}/man5/*.5*
> %dir %{_sysconfdir}/nagios
> %config(noreplace) %{_sysconfdir}/nagios/*
> 
> 1)I like more explicit file list, however that's me.
> 2) I guess %dir %{_sysconfdir}/nagios is owned by nagios-plugins
> and not needed in this package?

No, %{_sysconfdir}/nagios is not owned by any package that this package requires.

> Would be nice if you could create a FAS account and do a koji scratch build.

Yes, I will.

Thanks for your thoughts and comments :)

Comment 3 Thomas Spura 2011-11-14 21:50:01 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Would be nice if you could create a FAS account and do a koji scratch build.
> 
> Yes, I will.

Any news here? :)

As you are searching for a sponsor and this is your only review request so far, have you done some informal review requests yet?

For more information on how to get sponsored see:
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Submitting_quality_new_packages

Comment 4 Trond H. Amundsen 2011-11-15 10:27:02 UTC
As my attention was needed elsewhere lately, this was pushed to the background and I apologize for that. In the interest in moving things forward, I have addressed most of the issues in comment #1 and uploaded new versions of the spec file and SRPM here:

http://folk.uio.no/trondham/review2/nagios-plugins-openmanage.spec
http://folk.uio.no/trondham/review2/nagios-plugins-openmanage-3.7.3-2.el6.src.rpm

I have done a scratch build as requested:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3514976

I also include results from rpmlint for your convenience:

$ rpmlint nagios-plugins-openmanage.spec nagios-plugins-openmanage-3.7.3-2.el6.src.rpm 
nagios-plugins-openmanage.src: W: spelling-error Summary(en_US) plugin -> plug in, plug-in, plugging
nagios-plugins-openmanage.src: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
1 packages and 1 specfiles checked; 0 errors, 2 warnings.

In reply to comment #3, I'm afraid that I haven't done much of informal review requests yet, but I'll get to it as I'm still in need of a sponsor.

Comment 5 Trond H. Amundsen 2011-11-15 11:30:08 UTC
I forgot rpmlint on the binary RPM:

$ rpmlint nagios-plugins-openmanage-3.7.3-2.el6.x86_64.rpm 
nagios-plugins-openmanage.x86_64: W: spelling-error Summary(en_US) plugin -> plug in, plug-in, plugging
nagios-plugins-openmanage.x86_64: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
nagios-plugins-openmanage.x86_64: E: no-binary
nagios-plugins-openmanage.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 3 warnings.

The last two are normal for Nagios plugins, as the actual plugins are placed under %{_libdir} even if they are architecture independent.

Comment 6 Thomas Spura 2011-11-26 14:02:29 UTC
Review:

- name ok
- noarch not possible (maybe file a featurerequest at nagios to make it possible to install plugins into /usr/share/nagios/plugins?)
- BR/R ok
- BuildRoot and defattr could be left out, but as you are targeting el5: ok
- license ok
- no *.la
- rpmlint ignorable:
  $ rpmlint /home/tom/rpmbuild/SRPMS/nagios-plugins-openmanage-
    3.7.3-2.fc16.src.rpm /home/tom/rpmbuild/RPMS/x86_64/nagios-plugins-
    openmanage-3.7.3-2.fc16.x86_64.rpm
    nagios-plugins-openmanage.x86_64: E: no-binary
    nagios-plugins-openmanage.x86_64: W: only-non-binary-in-usr-lib
    2 packages and 0 specfiles checked; 1 errors, 1 warnings.
- no libs
- koji scratch build successful:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=3542984
- source match upstream:
  708257eedc7003d9c5fc3fba8200e572  check_openmanage-3.7.3.tar.gz
- files:
  * proper inclusion of man pages
  * %config there

NEEDSWORK:
- files:
  * %dir %{_sysconfdir}/nagios double owned:
    rpm -qf /etc/nagios
    nagios-3.2.3-11.fc16.x86_64

    This package R nagios-common, which doesn't R nagios, so the directory is
    unowned. It would be best to add the directory to nagios-common:
    Added to the list in bug 756839
    I wouldn't own it here and wait for nagios-common to pick it up...

- pre-build binary:
  https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
  Please delete the check_openmanage.exe binary in %prep, although you don't
  use them later on.

##############################################################

Let's wait for an answer of the nagios maintainer, but as your package looks 
fine otherwise and you made valid points in other reviews you are now:

##############################################################

SPONSORED

Comment 7 Trond H. Amundsen 2011-11-28 14:39:14 UTC
(In reply to comment #6)

> NEEDSWORK:
> - files:
>   * %dir %{_sysconfdir}/nagios double owned:
>     rpm -qf /etc/nagios
>     nagios-3.2.3-11.fc16.x86_64
> 
>     This package R nagios-common, which doesn't R nagios, so the directory is
>     unowned. It would be best to add the directory to nagios-common:
>     Added to the list in bug 756839
>     I wouldn't own it here and wait for nagios-common to pick it up...

The installed config file is just an example config with everything commented out. Also, the config file is optional and most users will probably not use it. In retrospect it shouldn't live in /etc/nagios as this directory should only contain actual Nagios config files, not config files for plugins.

In light of this I have removed it from /etc/nagios and only include it as documentation ("example.conf") instead.

> - pre-build binary:
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
>   Please delete the check_openmanage.exe binary in %prep, although you don't
>   use them later on.

Done.

Updated spec and SRPM available here:
http://folk.uio.no/trondham/review3/nagios-plugins-openmanage.spec
http://folk.uio.no/trondham/review3/nagios-plugins-openmanage-3.7.3-3.el6.src.rpm

Comment 8 Thomas Spura 2011-11-28 22:08:03 UTC
(In reply to comment #7)
> (In reply to comment #6)
> 
> > NEEDSWORK:
> > - files:
> >   * %dir %{_sysconfdir}/nagios double owned:
> >     rpm -qf /etc/nagios
> >     nagios-3.2.3-11.fc16.x86_64
> > 
> >     This package R nagios-common, which doesn't R nagios, so the directory is
> >     unowned. It would be best to add the directory to nagios-common:
> >     Added to the list in bug 756839
> >     I wouldn't own it here and wait for nagios-common to pick it up...
> 
> The installed config file is just an example config with everything commented
> out. Also, the config file is optional and most users will probably not use it.
> In retrospect it shouldn't live in /etc/nagios as this directory should only
> contain actual Nagios config files, not config files for plugins.
> 
> In light of this I have removed it from /etc/nagios and only include it as
> documentation ("example.conf") instead.

Great

> > - pre-build binary:
> >  
> > https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
> >   Please delete the check_openmanage.exe binary in %prep, although you don't
> >   use them later on.
> 
> Done.

Yepp.

Looks completely fine now.

###############################################################

APPROVED

###############################################################


You can now proceed and do a SCM request:
https://fedoraproject.org/wiki/Package_SCM_admin_requests

Information for adding the package to the git repository is at:
https://fedoraproject.org/wiki/New_package_process_for_existing_contributors
https://fedoraproject.org/wiki/Using_Fedora_GIT

Information for shipping the updates to EL and released fedora branches:
https://fedoraproject.org/wiki/PackageMaintainers/UpdatingPackageHowTo#Submit_your_update_to_Bodhi

Don't hesitate to ask (here or private mail), when you run into any problems.

Comment 9 Trond H. Amundsen 2011-11-29 09:14:04 UTC
New Package SCM Request
=======================
Package Name: nagios-plugins-openmanage
Short Description: Nagios plugin to monitor hardware health on Dell servers
Owners: trondham
Branches: f15 f16 el5 el6
InitialCC:

Comment 10 Gwyn Ciesla 2011-11-29 13:02:23 UTC
Git done (by process-git-requests).

Thomas, please take ownership of review BZs, thanks!

Comment 11 Fedora Update System 2011-12-01 10:06:00 UTC
nagios-plugins-openmanage-3.7.3-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.fc16

Comment 12 Fedora Update System 2011-12-01 10:07:13 UTC
nagios-plugins-openmanage-3.7.3-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.fc15

Comment 13 Fedora Update System 2011-12-01 10:08:02 UTC
nagios-plugins-openmanage-3.7.3-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.el6

Comment 14 Fedora Update System 2011-12-01 10:08:38 UTC
nagios-plugins-openmanage-3.7.3-3.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/nagios-plugins-openmanage-3.7.3-3.el5

Comment 15 Fedora Update System 2011-12-02 00:02:19 UTC
nagios-plugins-openmanage-3.7.3-3.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 16 Fedora Update System 2012-04-13 21:31:54 UTC
nagios-plugins-openmanage-3.7.3-3.fc15 has been pushed to the Fedora 15 stable repository.

Comment 17 Fedora Update System 2012-04-13 21:33:51 UTC
nagios-plugins-openmanage-3.7.3-3.fc16 has been pushed to the Fedora 16 stable repository.

Comment 18 Fedora Update System 2012-04-14 17:59:26 UTC
nagios-plugins-openmanage-3.7.3-3.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 19 Fedora Update System 2012-04-14 18:00:09 UTC
nagios-plugins-openmanage-3.7.3-3.el6 has been pushed to the Fedora EPEL 6 stable repository.


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