[Bug 182027] Review Request: postgrey

[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 report.

Summary: Review Request: postgrey


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=182027





------- Additional Comments From mfleming+rpm at enlartenment.com  2006-02-19 08:12 EST -------

NEEDSWORK

Review for release 1:
* RPM name is OK
* Source postgrey-1.24.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock (FC4)
* File list of postgrey looks OK
* Hasn't made my test instance explode.

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)

- Noticed some uses of %{_var} vs. %{_localstatedir} in there..

* rpmlint of postgrey:

rpmlint of postgrey-1.24-1.noarch.rpm
E: postgrey zero-length /etc/postgrey/postgrey_whitelist_clients.local
- %ghost this and make the user add it themselves once they've RTFM, IMO

E: postgrey non-standard-gid /var/run/postgrey postfix
E: postgrey non-standard-dir-perm /var/run/postgrey 0750
E: postgrey non-standard-uid /var/lib/postgrey postgrey
E: postgrey non-standard-gid /var/lib/postgrey postgrey
E: postgrey non-standard-dir-perm /var/lib/postgrey 0750
- These can be ignored IMHO

W: postgrey dangerous-command-in-%post chown
 -  Do it via %attr instead if you have to, I get the jitters when I see
    this.
W: postgrey service-default-enabled /etc/rc.d/init.d/postgrey
 -  Please don't give explosives to newbies. :-)

* The package should contain the text of the license
  (wiki: Packaging/ReviewGuidelines)
 - Yank a copy from an appropriate site, even as a SourceX if you want,
   Conversely, ask upstream to put it in the tarball.
  
* Missing dependancy on service for %postun (package initscripts)
* Missing dependancy on service for %preun (package initscripts)
* Missing dependancy on chkconfig for %post (package chkconfig)
* Missing dependancy on chkconfig for %preun (package chkconfig)
 - The Requires and pre/post install scriptlets need fixing.
 
 - Drop the PreReq entirely. Is this a holdover from MDK or something
   else?
 
 - Requires: postfix >= 2.1.0 (no policy server capability in older
   versions AFAIK. Again, no explosives for the newbs.)
   
 - For Requires(%post) etc, use the defaults from the Services part of
   http://www.fedoraproject.org/wiki/ScriptletSnippets  

Notes:
Other blockers:
- Don't use %define for Name/Version/Release, use them directly.
  It also makes the spec little cleaner. If possible avoid %defines at
  all unless you really need them.
- Use install rather than mkdir/cp for moving files around and creating your
  extra dirs.

"Would be nice if..."
- Any reason to move the defaults around? - upstream likes it's configs
  and spool in /etc/postfix & $postfix_spooldir respectively.  
- "Starting/Stopping $prog" in the init file looks a little ugly, when compared
  to other init files (not that most sysadmins *care*, but it stuck out for me
  :-)) Perhaps a simple "Starting/Stopping Postgrey:" would suffice?
- Toss a %{?dist} in Release as good practice and to make distro upgrades
  simpler and cleaner.

Stuff I liked:
- Generated manpages.


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.