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(a)redhat.com> 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.
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.
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....
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.