[Bug 702143] Review Request: wallaby - configuration service for Condor pools

[bugzilla at redhat.com]

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=702143

Tom "spot" Callaway <tcallawa at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tcallawa at redhat.com

--- Comment #3 from Tom "spot" Callaway <tcallawa at redhat.com> 2011-06-13 12:36:12 EDT ---
Initial Review:

* BuildRoot is not necessary, except on EPEL 5 and older.
* rm -rf %{buildroot} at the beginning of %install is not necessary, except on
EPEL 5 and older
* %clean section is not necessary if it is just rm -rf %{buildroot}, except on
EPEL 5 or older
* %defattr(-,root,root,-) is not necessary in %files sections, except on EPEL 5
or older.

All four of those items have sane defaults in current Fedora. :) 

* Given that this is a Fedora Hosted project, I would really like to see the
release tarball have a canonical upstream home, e.g. 
https://fedorahosted.org/releases/g/r/grid/%{name}-%{version}.tar.gz
Once that happens, the Source0 field in the spec file should reflect that.

* Is there a need for Requires: ruby if you have Requires: ruby(abi) = 1.8 ?
* Also, you have the BuildRequires split out (and duplicated) across subpackage
sections, this is unnecessary. BuildRequires apply to the SRPM only, so just
put them all at the top (once).
* Descriptions should be proper sentences and end in periods. Also, try to
flesh them out a bit. Two of your subpackages have identical descriptions.
* Strongly consider making a systemd unit file instead. SysVinit files are
deprecated. (https://fedoraproject.org/wiki/Packaging:Systemd)
* You're missing the exit 0 at the end of %pre (see
https://fedoraproject.org/wiki/Packaging:UsersAndGroups)
* Your %postun scriptlet seems entirely inappropriate. Compare it to the Fedora
standard here:
https://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_spec_file_scriptlets

Of course, you're going to move to a systemd unit file, right? :)

* Please read the guidelines concerning file and directory ownership:
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

I suspect you need to own some/all of the directories under %{ruby_sitelib}
that you're using. You may also be able to simplify the %files listings as a
result.

* Don't bother conditionalizing for Fedora newer than 12. At this point, you
can assume this package will never go into a Fedora older than 14. Just drop
those conditionals, or reframe them for RHEL if you want to put this in EPEL.

====

Show me a new package, and I'll do the final review.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.