Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 175748
Summary: | Review Request: cacti | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Mike McGrath <imlinux> |
Component: | Package Review | Assignee: | Aurelien Bompard <gauret> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-02-08 15:30:26 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Mike McGrath
2005-12-14 16:06:48 UTC
Needs work: * Use of buildroot is not consistant (wiki: PackagingGuidelines#UsingBuildRootOptFlags) * No downloadable source. Please give the full URL in the Source tag. * Group Application/System is not valid, it's "Applications/System" (with an s) * /usr/share/cacti/rra/.placeholder and the log file are chmod 750 because of the %attr definition in %files. The files should not be executable, so you should change the lines in %files to : %dir %attr(0755,cacti,cacti) %{_datadir}/%{name}/log/ %attr(0644,cacti,cacti) %{_datadir}/%{name}/log/* %dir %attr(0755,cacti,cacti) %{_datadir}/%{name}/rra/ %attr(0644,cacti,cacti) %{_datadir}/%{name}/rra/.placeholder * Is is necessary to make the log dir non world-readable ? I don't see important information there. Plus, the log file has to be readable by the webserver for the "View Log" function to work (in "System Utilities"). In the above example, I've set the dir 755 and the log file 644. * This is not FHS complient : - move include/config.php in /etc/cacti/config.php - move log/ to /var/log/cacti - move rra to /var/lib/cacti/rra/ And symlink the files back in /usr/share/cacti I would also move scripts/ to /var/lib/cacti/scripts, since it is very common for cacti users to add scripts. What do you think ? * In %prep, use %setup -q to avoid file listing * Please add a logrotate file for the log. Example: /var/log/cacti/cacti.log { monthly notifempty missingok compress create 0644 cacti cacti } * File listed twice: /usr/share/cacti/include/config.php * Missing dependancy on /sbin/service for %postun (package initscripts) * /usr/bin should be changed to %{_bindir} * Why "cp -avx" ? cp -a is enough * Don't hardcode uid 329 in useradd, it may be already used. Just let useradd find an available uid * You need mailx and MTA as Requires(post) because of the mail in %post * Changed Prereq to Requires(pre) * No need for php in %pre, you can remove it from Requires(pre) I don't disagree with any of these. I'll post the changes soon. I guess I was being a little loverly parrinoid about the log files. Going through my own I haven't seen anything too important. I've made these updates. Couple of issues: Simply running config.php out of /etc/ does not work as there are includes that fail, and copying the entire include directory seems inapproperate. I could write a patch? In the meantime I made a link from the config.php to /etc/cacti/config.php so it can at least be found there. Any thoughts? I'm also curious about others thoughts on running this as a Cacti user. Would it be more approperate to just run it as Apache? Either way the spec and srpm are both updated and in their original spots: Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-2.src.rpm When you update a SRPM, please always increment the Release number, do not keep it the same. > Simply running config.php out of /etc/ does not work as there are includes > that fail, and copying the entire include directory seems inapproperate. I > could write a patch? If it's very easy and obvious, yes, write a patch please. Users should not have to edit configuration files in /usr/share > I'm also curious about others thoughts on running this as a Cacti user. Would > it be more approperate to just run it as Apache? You mean the cron job ? I think it's best not to run it as the apache user, so that the apache user does not have write access to the RRA files in case of compromise (and there have been such vulnerabilities in cacti's history) * in %pre : /usr/sbin -> %{_sbindir} * /var -> %{_localstatedir} in the logrotate file * mailx and MTA should be in Requires(post) * /sbin/service should be in Requires(postun) * Using %doc in %files does the copying of files, so you don't have to do it in %install Minor: * the scripts don't have to be owned by the cacti user, but that's not a problem (it just spits out more warnings in rpmlint) * you don't _have_ to use %{__install}, %{__mkdir}, %{__cp} and %{__chmod}, using install, mkdir, etc... is enough. Do what you prefer, it's up to you. Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-3.noarch.rpm I've updated many of those changes (including minor). I ended up taking the mail out of the RPM all together, I don't remember getting emails from other packages saying "hey configure me here" I've taken the database config portion out of the config.php file and placed it in /etc/cacti/db.php It seems like nothing else in config.php would need to be edited for normal use. Thanks for all your help on this, I plan on submitting more packages in the future so feel free to straighten me out on things that are not best practice. > I ended up taking the mail out of the RPM all together, I don't > remember getting emails from other packages saying "hey configure me here" Agreed. Seemed "unnatural" to me too. > I've taken the database config portion out of the config.php file and placed > it in /etc/cacti/db.php It seems like nothing else in config.php would need > to be edited for normal use. Very nice. There are a few additional config vars for some HTML colors in config.php, but we can ignore this. Actually, I wish upstream had splitted the db config out of config.php. Could you submit your patch upstream (with the /etc/cacti/db.php paths replaced by include/db.php) ? A few last things : * remove /sbin/service from Requires (it's already in Requires(postun)) * in %install, first you %__install the .placeholder file, then you empty it with echo. Why ? I think that is file is here just to prevent winzip from not including the rra dir, so you should not bother about it (I think you can even remove it entirely). These changes are ready, I've submitted an include/db.php patch to the Cacti team. Also while looking for their bug system I found 4 patches for 0.8.6g and have included them in this release. Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-4.src.rpm * in the cron job, replace /usr/bin by %{_bindir} * to make it clear that the patches come from upstream, please give the full url (ex: http://www.cacti.net/downloads/patches/0.8.6g/short_open_tag_parse_error.patch) They won't start with "cacti-0.8.6g-", but it is more important to be able to verify them against upstream. * drop the empty %build section * concerning the apache restart in the scriptlets : 1. we want to condrestart apache on install, in order for our new cacti.conf to be taken into account. 2. we want to condrestart apache on upgrade, in case we changed cacti.conf 3. we want to condrestart apache on uninstall, so that apache sees that the cacti.conf file has been removed. For 1 we need a post scriptlet like this : %post if [ $1 == 1 ]; then /sbin/service httpd condrestart >/dev/null 2>&1 fi We can combine 2 and 3 into a simple %postun scriptlet without tests : %postun /sbin/service httpd condrestart >/dev/null 2>&1 See http://fedoraproject.org/wiki/ScriptletSnippets for more info on which tests to run We are having a larger problem now. SELinux blocks httpd from accessing /var/log and /var/lib. The RRAs are correctly created by the poller script, but can't be seen in the web interface. I'm posting in fedora-selinux-list to get advice. I've got these changes in. I'll wait to post them until we can figure out what to do about SELinux. I'll be monitoring the thread: https://www.redhat.com/archives/fedora-selinux-list/2005-December/msg00124.html Spec Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti.spec SRPM Url: http://preview.iesabroad.org/~mmcgrath/cacti/cacti-0.8.6g-6.src.rpm I've made the changes above and used the "quick hack" from the SELinux thread. I'm sure there's got to be a more approperate way to do this. Any ideas anyone? Welp, selinux and FHS don't always get along :-D I've moved all the log files and associated web files to /var/www/cacti/ until we come up with a better way to do it. SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6g-7.src.rpm SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6h-3.src.rpm Changes: -Moved log files back to /var/log/cacti -Moved Scripts back to /var/lib/cacti/scripts -Moved rra files back to /var/lib/cacti/rra -Changed logfile to (664,cacti,apache) so the "clear log file" will work -Upstream had a new version come out -Upstream already had 4 patches for that version SELinux suggestion made by Paul Howarth sounds very reasonable: https://www.redhat.com/archives/fedora-extras-list/2006-January/msg01169.html If Cacti gets approved I'll submit a bug report to SELinux and try to get it incorperated into the policy. I'll use the FC5 modules once FC5 comes out (if it applies) Sorry for not getting back earlier. In you last spec file, seem to have moved files back to /usr/share and /var/lib. In order to avoid the SELinux problem, I think we should keep everything in /var/www, as you said in comment 12. I can move it back but do you see any reason Paul's suggestion won't work? (From Comment 13) https://www.redhat.com/archives/fedora-extras-list/2006-January/msg01169.html OK, it looks good. Minor improvements: - add " || : " at the end of the scriptlets to make sure they always return 0 - the logrotate file contains a "//" in the logfile path - please add a README.Fedora with something like that : If you have SELinux enabled, you have to run the following commands : chcon -R -t httpd_sys_content_t /var/log/cacti/ chcon -R -t httpd_sys_content_t /var/lib/cacti/rra/ This README could be removed when cacti is added into the policy. If you agree with the above, make the changes and I'll approve it Sounds good to me, all things added: %changelog * Mon Feb 6 2006 Mike McGrath <imlinux> - 0.8.6h-4 - Fixed some scriptlets to always return 0 - Fixed extra '/' in logrotate - Added README.Fedora SPEC: http://mmcgrath.net/~mmcgrath/cacti/cacti.spec SRPM: http://mmcgrath.net/~mmcgrath/cacti/cacti-0.8.6h-4.src.rpm Review for release 4: * RPM name is OK * Source cacti-0.8.6h.tar.gz is the same as upstream * This is the latest version * Builds fine in mock * rpmlint looks OK * File list looks OK * Works fine APPROVED Again, sorry for keeping you waiting. Please post here the bug number for SELinux integration when you file it. I've created an SELinux bug: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=180482 Also cacti is now available in devel. (Soon FC3 and FC4) |