Just some quick comments:

- Please check the guidelines for the python macros
  https://fedoraproject.org/wiki/Packaging:Python#Macros
- A simple comment in %build (like #nothing to build) to calm rpmlint down is sufficient.
- Python 2.7.x was introduced around F14/F15. Maybe drop the Python 2.6.x stuff.
- %clean and BuildRoot tag can be dropped if the packages is not going to EPEL5. If it's going to EPEL5 then you have to add %defattr to the %files section.

Hi Fabian,

thanks for the comments. Here is what I've updated in the spec file.

 - removed custom python macros and now using python_sitelib
 - using __python macro to invoke python interpreter
 - removed echo from %build. Added comment (# nothing to build)
 - removed %if "%{py_version}" == "2.6" condition
 - removed %clean and BuildRoot

Thanks!
-Lukas

Hello,

is there anything I can do to push this forward ?

Thanks!
-Lukas

I'll take it for formal review.

(In reply to comment #4)
> I'll take it for formal review.

Eric, please continue with the review, I will sponsor Lukas afterwards.

Thanks Dan.  Finally getting the review going today.

Ok, editorial nitpicks:

> System Storage Manager provides easy to use command line interface to manage

"provides an easy to use ..."

> technologies via single unified interface.

"technologies via a single unified interface."

> You should install the ssm if you need

Should probably say "You should install system-storage-manager if you need" ...

And maybe 2 spaces (vs 1) after every sentence in the %description?

Now I'll go point by point through review guidelines to make sure I'm not missing anything.  Pretty simple specfile though!

All this is OK (the MUSTS):

rpmlint is amazingly quiet ;)
package naming is fine
specfile name matches
AFAICT, it follows python packaging rules, but I am no expert there!
License (GPLv2+) is acceptable for Fedora.
License matches license statements in source
COPYING file contains license and is in %doc
Specfile is legible and in American e=English
Source matches upstream
Upstream source URL is fine
package built fine: http://koji.fedoraproject.org/koji/taskinfo?taskID=4180107
RPM does own the two dirs it creates
File perms seem fine
Macros are consistently used in the specfile
Package contains permissible content (code)
Doc files work properly with --excludedocs; there is no %doc directive but %{_docdir} seems to imply that.
No static or devel libraries, so no issues there.


Questions / suggestions:

For runtime requires, I wonder about the fs utilities it invokes.  If it handles it gracefully when they are missing, probably no need for a runtime requirement,  but if not, perhaps you should require those packages at least for now?

I'll try to grep through & see what all might get executed.

Ok, just looking at "command = " lines, we have these things being executed as commands (this is a little crude, not all are actually system commands I think)

blkid
btrfs
cryptsetup
device
dmsetup
filesystem
fsck.{0}
lvcreate
lvm
lvremove
lvresize
mkfs.{0}
mkfs.btrfs
mount
pvremove
remove
resize
resize2fs
subvolume
tune2fs
umount
vgcreate
vgextend
vgreduce
vgremove
which
wipefs
xfs_check
xfs_db
xfs_growfs

which seems to correspond to these packages:

cryptsetup-luks
device-mapper
e2fsprogs
lvm2
util-linux-ng
xfsprogs

Perhaps these should be explicitly listed as runtime requirements until you are sure that it can function reasonably without them in place?

Hm package list was from an older system, so s/util-linux-ng/util-linux/ at least.

And "which" (from the "which" package) is also invoked.

I'm not sure if things like util-linux & which must be listed as Requires; they are not needed for BuildRequires since they are included in the minimum build environment, but I can't find a similar exception list for Requires... will let you know.

Ok, I've confirmed that even which & util-linux should be listed as Requires:

Hi Eric,

thank you very much for the review! I have updated description according your comments in Comment 7.

Regarding the commands used in the system storage manager:

cryptsetup-luks - is not required and if not present crypt backend will not be used. It has been designed this way.

device-mapper (dmsetup) - it is required only by crypt backend and again if it is not present, it will not be used.

lvm2 - it is not required and if not present lvm backend will not be used. It has been designed this way.

btrfs-progs - it is not required and if not present btrfs backend will not be used. It has been designed this way.

util-linux - it already is required in the spec file

which - This have to be added into the required packages. Thanks for pointing this out.

xfsprogs
e2fsprogs
   - Unfortunately I've completely forgot about those and the system storage manager will not gracefully handle the situation when the file system tools are missing. We are already working on a patch, however it might make more sense to just require those two packages since it will probably cover most of the usual setups anyway. I am going to add those packages to required for now.


Spec URL: http://people.redhat.com/lczerner/files/system-storage-manager.spec
SRPM URL: http://people.redhat.com/lczerner/files/system-storage-manager-0.2-1.fc16.src.rpm


Thanks Eric!
-Lukas

Thanks for the updates Lukas, looks good to me.

New Package SCM Request
=======================
Package Name: system-storage-manager
Short Description: A single tool to manage your storage
Owners: lczerner
Branches: 
InitialCC:

"lczerner" is not in the packager group.

Eric,

as my sponsor could you add me into the packager group ?

Thanks!
-Lukas

Git done (by process-git-requests).

Package Change Request
======================
Package Name: system-storage-manager
New Branches: f18
Owners: lczerner

(In reply to comment #19)
> Package Change Request
> ======================
> Package Name: system-storage-manager
> New Branches: f18
> Owners: lczerner

I need the f18 branch to request the bodhi update to add this new package to the updates-testing.

Thanks!
-Lukas

Git done (by process-git-requests).

system-storage-manager-0.2-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/system-storage-manager-0.2-1.fc18

system-storage-manager-0.2-1.fc18 has been pushed to the Fedora 18 testing repository.

system-storage-manager-0.2-1.fc18 has been pushed to the Fedora 18 stable repository.