Bug 514602 - Review Request: system-config-audit - an utility for editing audit configuration
Summary: Review Request: system-config-audit - an utility for editing audit configuration
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jochen Schmitt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-07-29 19:49 UTC by Miloslav Trmač
Modified: 2009-09-07 12:20 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-09-07 12:20:38 UTC
Type: ---
Embargoed:
jochen: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Miloslav Trmač 2009-07-29 19:49:11 UTC
Spec URL: http://people.redhat.com/mitr/packaging/system-config-audit.spec
SRPM URL: http://people.redhat.com/mitr/packaging/system-config-audit-0.4.11-1.src.rpm
Description: An utility for editing audit configuration.

Comment 1 Bill Nottingham 2009-07-30 17:04:35 UTC
1) usermode. Ick.

2) This needs serious UI work, the spacing is all fubar.

3) Who exactly is the intended target of this? Our general admins don't want to use X based tools. Our desktop users don't care.

Comment 2 Steve Grubb 2009-07-30 17:08:43 UTC
Just 3 issues I labeled with a letter in case discussion is needed. Looking
over the spec file:
A) release should be 1%{?dist}

I get the following errors from rpmlint:

B) system-config-audit.src: E: no-cleaning-of-buildroot %install
C) system-config-audit.src: E: no-buildroot-tag

Comment 3 Miloslav Trmač 2009-07-30 17:22:46 UTC
Thanks for the review.

(In reply to comment #2)
> Just 3 issues I labeled with a letter in case discussion is needed. Looking
> over the spec file:
> A) release should be 1%{?dist}
Dist tag usage is not mandatory: https://fedoraproject.org/wiki/Packaging:DistTag#Do_I_Have_To_Use_the_Dist_Tag.3F

> I get the following errors from rpmlint:
> 
> B) system-config-audit.src: E: no-cleaning-of-buildroot %install
> C) system-config-audit.src: E: no-buildroot-tag  
These are done automatically by recent RPM versions:

https://fedoraproject.org/wiki/PackagingGuidelines#BuildRoot_tag
https://fedoraproject.org/wiki/PackagingGuidelines#Prepping_BuildRoot_For_.25install

Comment 4 Jochen Schmitt 2009-07-30 17:51:44 UTC
Good:
+ Basename of the SPEC file matches with package name
+ Package has a proper project homepage
+ Could downloading source via spectool -g
* Consistently usage of rpm macros
+ Package tarball matches with upstream 
(md5sum: 2f47c6549b6adee06126243ec2162cb9)
+ Package contains no subpackages
+ Package has a valid licen tag
+ The license tag states GPLv2 as a valid OSS license
+ Copyrigh notes on headers matches with license tag
+ Package contains a verbatin copy of the license text
+ Local build works fine
+ Package support SMP build
+ Package use %find_lang macro
+ Rpmlint is silent on binary rpm.
+ Debunginfo package contains source files
+ Koji build works fine
+ Files has proper permisions
+ All packaged files are owned by the package
+ No packaged file belong to another package
+ %Ddoc is small, so we don't need a separate subpackage
+ Package contains proper %ChangeLog

Bad:
- Package contains not a BuildRoot tag.
- Buildroot will not been clean on the start of %clean and %install
- The package description could be more verbose.
  As a minimum you should wrote:
    this package provide a GUI which allows the user to
    configure the linux audit subsystem.
- Please create a conflict statement agains the audit package
  before the splitt off.

Comment 5 Miloslav Trmač 2009-07-30 21:07:57 UTC
Thanks for the review.
(In reply to comment #4)
> Bad:
> - Package contains not a BuildRoot tag.
https://fedoraproject.org/wiki/PackagingGuidelines#BuildRoot_tag says it is not necessary any more.
> - Buildroot will not been clean on the start of %clean and %install
Same here - https://fedoraproject.org/wiki/PackagingGuidelines#Prepping_BuildRoot_For_.25install for %install.
%clean does clean the buildroot.

> - The package description could be more verbose.
>   As a minimum you should wrote:
>     this package provide a GUI which allows the user to
>     configure the linux audit subsystem.
Thanks, fixed.

> - Please create a conflict statement agains the audit package
>   before the splitt off.  
AFAICS this should not be necessary - system-config-audit was a separate subpackage of audit.  There are no file conflicts between the old audit package and any system-config-audit package, I can't see why the old audit package and a new system-config-audit package could not coexist.

Updated package:
http://people.redhat.com/mitr/packaging/system-config-audit.spec
http://people.redhat.com/mitr/packaging/system-config-audit-0.4.11-2.src.rpm

Comment 6 Steve Grubb 2009-08-07 20:37:02 UTC
What's the status on this bug? This is blocking release of audit-2.0. Thanks.

Comment 7 Jochen Schmitt 2009-08-09 18:15:53 UTC
OK; I have take a short look on it and it seems to be ok, so I can approve your package.

Comment 8 Miloslav Trmač 2009-08-10 12:26:28 UTC
New Package CVS Request
=======================
Package Name: system-config-audit
Short Description: Utility for editing audit configuration
Owners: mitr
Branches: 
InitialCC:

Comment 9 Kevin Fenzi 2009-08-11 05:03:35 UTC
cvs done.


Note You need to log in before you can comment on or make changes to this bug.