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.
# 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.
(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 :)
(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
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.
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.
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
(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
(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.
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:
Git done (by process-git-requests). Thomas, please take ownership of review BZs, thanks!
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
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
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
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
nagios-plugins-openmanage-3.7.3-3.el6 has been pushed to the Fedora EPEL 6 testing repository.
nagios-plugins-openmanage-3.7.3-3.fc15 has been pushed to the Fedora 15 stable repository.
nagios-plugins-openmanage-3.7.3-3.fc16 has been pushed to the Fedora 16 stable repository.
nagios-plugins-openmanage-3.7.3-3.el5 has been pushed to the Fedora EPEL 5 stable repository.
nagios-plugins-openmanage-3.7.3-3.el6 has been pushed to the Fedora EPEL 6 stable repository.