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=721144 --- Comment #2 from Chris Lalancette <clalance@xxxxxxxxxx> 2011-07-19 16:33:28 EDT --- (In reply to comment #1) > Looks good overall. I've only a bunch of fairly minor comments > > rpmlint shows no problems > > $> rpmlint imagefactory.spec > 0 packages and 1 specfiles checked; 0 errors, 0 warnings. > $> rpmlint imagefactory-0.3.1-1.fc14.src.rpm > 1 packages and 0 specfiles checked; 0 errors, 0 warnings. > > other comments: > > - use %{version} instead of 0.3.1 in the source URL Fixed. > > - license is 'ASL 2.0' not GPLv2 Fixed. > > - looking at rpm's GROUPS file, I think I'd go for Applications/System > instead of Development/Libraries Fixed. > > - URL instead of Url Fixed. > > - I don't think euca2ools is actually required; we run all the euca- > commands inside EC2 instances not locally When we are doing "upload" or "local" style builds, the euca2ools are actually required to do the bundling. See imagefactory/builders/FedoraBuilder.py, around lines 1383. > > - it also doesn't look like python-suds is a direct dependency; psphere > needs it alright, but not imagefactory directly Right, removed. > > - no need for python BR, python-setuptools is enough Sure, removed. > > - use %{__python} instead of python Done. > > - like in psphere, just do: > > %install > %{__python} setup.py install --root=$RPM_BUILD_ROOT --skip-build Done. > > - need to require chkconfig for post and preun and initscripts for preun > > http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets Fixed. > > - looks like you also forgot %{name} in the service stop line i.e. > > - /sbin/service stop >/dev/null 2>&1 > - /sbin/service %{name} stop >/dev/null 2>&1 Oops. Fixed. > > - I don't think condrestart in %postun is appropriate in our case; e.g. > we don't want to cancel running builds or pushes when doing an update. > For example, koji doesn't do this Good point. I guess that means we remove the %postun section completely. It does make me a tad nervous, in that after a security update the user is still running the old, affected code. Still, this is far from the only place that has this problem. Removed. > > - Use %{_initddir} not %{_initrddir} > > http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem Fixed. New spec: http://people.redhat.com/clalance/imagefactory/imagefactory.spec New SRPM: http://people.redhat.com/clalance/imagefactory/imagefactory-0.3.1-2.fc14.src.rpm Thanks for the 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review