Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: shinken - python monitoring tool
https://bugzilla.redhat.com/show_bug.cgi?id=705108
Summary: Review Request: shinken - python monitoring tool Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: david.hannequin@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Story Points: ---
Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6-1.fc15.src.rpm Description: The main goal of Shinken is to allow users to have a fully flexible architecture for their monitoring system that can easily scale to large environments.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |volker27@gmx.at
--- Comment #1 from Volker Fröhlich volker27@gmx.at 2011-05-19 17:11:28 EDT --- You can use the name macro in the tarball name.
The license is AGPLv3+. Rpmlint still complains, because the license was forgotten in the list of acceptable licenses. It is included by now.
Requiring a Python newer or equal to 2.6 is not necessary, as all Fedora versions have it. If you consider maintaining it in EPEL 5 as well, you must add a clean section and rm -rf the buildroot in the install section. In that case please also pick a BuildRoot syntax from http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag
If you don't want to maintain in EPEL, drop the buildroot definition. Besides that, FOR_PACKAGERS points out Python 2.4 would also work.
Rather use %global than %define.
defattr seems unnecessary now: https://fedorahosted.org/fpc/ticket/77 But if you use it, add the minus in the end: %defattr(-,root,root,-)
Drop the -n %{name}-%{version} from the setup macro. It's not necessary. You also don't have to define the __python macro.
It's good practice to backup patched files like: %patch1 -p1 -b .something_that_identifies_the_patch
Don't use the install macro. Just use the normal command. Probably put a few comments to make everything clear. Especially when you decide to do other than obvious.
Why do you need to have executable __init__.py (chmod)?
The unitdir macro is not defined. Thus a rebuild fails.
The paths inside FROM_NAGIOS_TO_SHINKEN don't reflect the situation for Fedora. Other files may be affected as well. Please check the paths in the config files, whether the targets exist and are sane. I can see /usr/local/...
README is only a symlink pointing towards README.rst -- no need to include it.
Any chance to run the unit tests?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #2 from David Hannequin david.hannequin@gmail.com 2011-05-20 09:20:41 EDT --- Hi,
Unitdir macro is defined in the file /etc/rpm/macros.systemd. I corrected the other errors before putting a new spec file.
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #3 from David Hannequin david.hannequin@gmail.com 2011-05-24 17:54:18 EDT --- Hi,
New spec file and SRPM : Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.4-1.fc15.src.rpm
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #4 from Volker Fröhlich volker27@gmx.at 2011-05-26 16:41:43 EDT --- That means, unitdir is only defined in F15 and up. Thus all the buildroot stuff is either unnecessary, as you only want to support F15 and up or you have to find an alternative for these cases.
Please keep an accurate changelog and also address the other comments so far.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #5 from David Hannequin david.hannequin@gmail.com 2011-05-29 13:29:06 EDT --- Hi,
I want to package Shinken only in Fedora 15 and higher.
I now use %global instead of %define.
There is no patch for this release but I keep in mind your remark about the - b of the patch command.
I removed the macro commands to install and others.
FROM_NAGIOS_TO_SHINKEN the file is not included in the RPM. I do not understand your comment. The files are specific to Fedora fedora folder.
New spec file and SRPM : Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.4-2.fc15.src.rpm
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #6 from Volker Fröhlich volker27@gmx.at 2011-05-29 16:16:16 EDT --- The buildroot definition is also unnecessary then.
Please take a look at the result of licensecheck:
... shinken/contactdowntime.py: AGPL (v3 or later) shinken/bin.py: AGPL (v3 or later) shinken/db_oracle.py: AGPL (v3 or later) shinken/log.py: AGPL (v3 or later) ...
All the files say AGPLv3+ -- not AGPLv3.
Why don't you include FROM_NAGIOS_TO_SHINKEN? Is it not relevant to users of Shinken?
With "changelog" I meant what's inside the spec file. Just by looking at the changelog "you fixed something and you replaced some macro". Just put it there like you put it above.
What about the unit tests?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #7 from David Hannequin david.hannequin@gmail.com 2011-05-29 16:46:20 EDT --- Hi,
What do you mean "What about the unit tests?" ? Shinken RPMs work fine and i tested on Fedora 15. You can see -> http://hvad.fedorapeople.org/fedora/shinken/shinken.txt.
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #8 from David Hannequin david.hannequin@gmail.com 2011-05-30 11:55:10 EDT --- Hi,
For now, there is no unit test in the rpm Shinken because the author did not create a test section in the file setup.py. After some discussion with him we'll add a test section in the file setup.py in the future version.
In addition there are tests that are already visible on this and realize url http://shinken-monitoring.de:8080/ and I also test the RPM version also works on Fedora 15.
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #9 from David Hannequin david.hannequin@gmail.com 2011-05-31 03:16:29 EDT --- Hi,
New spec file and SRPM : Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.4-3.fc15.src.rpm
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #10 from Volker Fröhlich volker27@gmx.at 2011-06-24 18:08:51 EDT --- Please adapt the Requires for the sub-packages as described here: http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
Put the common Requires up to the base package, instead of repeating them over and over for each sub-package.
Explicitly naming Python is not necessary, as, for instance, python-pyro requires it.
Putting the Requires on one line each is a bit more readable.
Eventually delete the useless BuildRoot or correct it according to: http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag
The Python macro is useless, as you can see here: http://fedoraproject.org/wiki/Packaging:Python#Macros
Also use the python macro instead of "/usr/bin/python".
There are some occurrences of "shinken" in the files section, where you should use the name macro.
Your base package states "%{_libdir}/nagios/plugins". Nevertheless, it doesn't require Nagios and would also claim ownership of that directory, I suppose. Probably these files should go to one of your sub-packages?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #11 from David Hannequin david.hannequin@gmail.com 2011-07-01 17:28:00 EDT --- Hi,
Requires in the sub packages have been removed and i putting the Requires on one line each.
After discussion with the developer of Shinken, Shinken plugins can be place on Nagios Plugins but it's possible to put in an other directory.
Shinken was released in version 0.6.5. I still have tests to do before uploading the SRPM and SPEC.
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #12 from David Hannequin david.hannequin@gmail.com 2011-09-18 12:07:08 EDT --- Hi,
New spec file and SRPM with fix : Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.6.5-1.fc15.src.rpm
Best regard ps : In a few months of Shinken version 0.8 will come out with a WebUI
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #13 from David Hannequin david.hannequin@gmail.com 2011-11-15 15:03:17 EST --- Hi,
New spec file and SRPM with fix : Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec
SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-0.8.1-1.fc15.src.rpm
Sub-package Shinken poller requires the Nagios plugins as it uses them to make checks.
Best regard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
Jorge A Gallegos kad@blegh.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kad@blegh.net
--- Comment #14 from Jorge A Gallegos kad@blegh.net 2012-04-02 15:02:31 EDT --- (In reply to comment #11)
Hi,
Requires in the sub packages have been removed and i putting the Requires on one line each.
After discussion with the developer of Shinken, Shinken plugins can be place on Nagios Plugins but it's possible to put in an other directory.
Shinken was released in version 0.6.5. I still have tests to do before uploading the SRPM and SPEC.
Best regard
I think most of the issues have been worked out for this spec? Some comments:
- I'd still change %{_libdir}/nagios to %{_libdir}/shinken, because that directory is already owned by the nagios package I think - You're sed'ing /usr/bin/python as the interpreter on several files, I would put /usr/bin/env python instead (not sure if shinken is python3 compatible or etc) - Volker was probably referring to that python_sitelib macro you have on top, check http://fedoraproject.org/wiki/Packaging:Python#Macros, since you stated you want shinken to be supported in fedora 15+: "In Fedora 13 and greater, the following macros are defined for you: ... snip... python_sitelib" - Also, seems like shinken is up to v1.0.1 (http://www.shinken-monitoring.org/download/) so maybe you should update from upstream too?
Otherwise, very exciting! I'd like to see shinken in fedora some time soon!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=705108
--- Comment #15 from David Hannequin david.hannequin@gmail.com 2012-04-09 09:41:08 EDT --- Hi,
New spec file and SRPM with fix : Spec URL: http://hvad.fedorapeople.org/fedora/shinken/shinken.spec
SRPM URL: http://hvad.fedorapeople.org/fedora/shinken/shinken-1.0.1-1.fc16.src.rpm
I update from upstream and fix some issues.
Sorry but shinken isn't compatible with python 3.
Best regard
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla