Bug 721144 - Review Request: imagefactory - System image generation tool
Summary: Review Request: imagefactory - System image generation tool
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Mark McLoughlin
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 717966
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-13 20:33 UTC by Chris Lalancette
Modified: 2013-08-29 14:55 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-07-20 17:47:53 UTC
Type: ---
Embargoed:
markmc: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Chris Lalancette 2011-07-13 20:33:42 UTC
Spec URL: http://people.redhat.com/clalance/imagefactory/imagefactory.spec
SRPM URL: http://people.redhat.com/clalance/imagefactory/imagefactory-0.3.1-1.fc14.src.rpm

Description:
imagefactory is a daemon that listens for build requests via QMF.  When
a build request is received, imagefactory will build the requested image
for the requested target.

Comment 1 Mark McLoughlin 2011-07-18 08:47:59 UTC
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

 - license is 'ASL 2.0' not GPLv2

 - looking at rpm's GROUPS file, I think I'd go for Applications/System
   instead of Development/Libraries

 - URL instead of Url

 - I don't think euca2ools is actually required; we run all the euca-
   commands inside EC2 instances not locally

 - it also doesn't look like python-suds is a direct dependency; psphere
   needs it alright, but not imagefactory directly

 - no need for python BR, python-setuptools is enough

 - use %{__python} instead of python

 - like in psphere, just do:

    %install
    %{__python} setup.py install --root=$RPM_BUILD_ROOT --skip-build

 - need to require chkconfig for post and preun and initscripts for preun

http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets

 - 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

 - 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

 - Use %{_initddir} not %{_initrddir}

http://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem

Comment 2 Chris Lalancette 2011-07-19 20:33:28 UTC
(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.

Comment 3 Mark McLoughlin 2011-07-20 08:46:51 UTC
(In reply to comment #2)
> (In reply to comment #1)
> >  - 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.

Ah, of course - right

I was just about to approve this, but then I remembered to run rpmlint on the actual RPM itself:

$> rpmlint imagefactory-0.3.1-2.fc14.noarch.rpm
imagefactory.noarch: E: explicit-lib-dependency python-httplib2
imagefactory.noarch: W: conffile-without-noreplace-flag /etc/pki/imagefactory/cert-ec2.pem
imagefactory.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/imagefactory/ApplicationConfiguration.py 0644L /usr/bin/env
imagefactory.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/imagefactory/Template.py 0644L /usr/bin/env
imagefactory.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/imagefactory/ImageWarehouse.py 0644L /usr/bin/env
imagefactory.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/imagefactory/VMWare.py 0644L /usr/bin/env
imagefactory.noarch: W: no-manual-page-for-binary imgfac.py
1 packages and 0 specfiles checked; 5 errors, 2 warnings.


Re: explicit-lib-dependency - this looks like a false positive to me. See http://lists.fedoraproject.org/pipermail/devel/2010-March/133955.html

Re: conffile-without-noreplace-flag - do we allow users to replace this with their own cert? If so, the error is valid.

Re: non-executable-script - do sed -i '/\/usr\/bin\/env python/d' imagefactory/*.py

Re: no-manual-page-for-binary - there actually is a man page, but not with the same name as the binary. Do we really want the binary to be installed as imgfac.py ?

Comment 4 Chris Lalancette 2011-07-20 14:49:51 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > >  - 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.
> 
> Ah, of course - right
> 
> I was just about to approve this, but then I remembered to run rpmlint on the
> actual RPM itself:
> 
> $> rpmlint imagefactory-0.3.1-2.fc14.noarch.rpm
> imagefactory.noarch: E: explicit-lib-dependency python-httplib2
> imagefactory.noarch: W: conffile-without-noreplace-flag
> /etc/pki/imagefactory/cert-ec2.pem
> imagefactory.noarch: E: non-executable-script
> /usr/lib/python2.7/site-packages/imagefactory/ApplicationConfiguration.py 0644L
> /usr/bin/env
> imagefactory.noarch: E: non-executable-script
> /usr/lib/python2.7/site-packages/imagefactory/Template.py 0644L /usr/bin/env
> imagefactory.noarch: E: non-executable-script
> /usr/lib/python2.7/site-packages/imagefactory/ImageWarehouse.py 0644L
> /usr/bin/env
> imagefactory.noarch: E: non-executable-script
> /usr/lib/python2.7/site-packages/imagefactory/VMWare.py 0644L /usr/bin/env
> imagefactory.noarch: W: no-manual-page-for-binary imgfac.py
> 1 packages and 0 specfiles checked; 5 errors, 2 warnings.
> 
> 
> Re: explicit-lib-dependency - this looks like a false positive to me. See
> http://lists.fedoraproject.org/pipermail/devel/2010-March/133955.html

Yeah, I figured it was just looking for the lib string, and that post confirms it :).

> 
> Re: conffile-without-noreplace-flag - do we allow users to replace this with
> their own cert? If so, the error is valid.

No, the user can't replace it.  It is the global ec2 certificate, comes from amazon, and is used (in some way) to sign the bundled images.  As such, I believe it is correct as-is.

> 
> Re: non-executable-script - do sed -i '/\/usr\/bin\/env python/d'
> imagefactory/*.py

Fixed.

> 
> Re: no-manual-page-for-binary - there actually is a man page, but not with the
> same name as the binary. Do we really want the binary to be installed as
> imgfac.py ?

This has been a source of annoyance to me for some time.  Upstream, I think we should eventually rename the binary to imagefactory and fix all of the references to it, but I don't think I want to make that kind of change right at the moment.

I've uploaded revision 3:

SRPM: http://people.redhat.com/clalance/imagefactory/imagefactory-0.3.1-3.fc14.src.rpm
SPEC: http://people.redhat.com/clalance/imagefactory/imagefactory.spec

Comment 5 Mark McLoughlin 2011-07-20 14:56:31 UTC
Okay, looks good to me

Comment 6 Chris Lalancette 2011-07-20 15:13:02 UTC
Thanks!

Comment 7 Chris Lalancette 2011-07-20 15:13:57 UTC
New Package SCM Request
=======================
Package Name: imagefactory
Short Description: System image generation tool
Owners: clalance

Comment 8 Gwyn Ciesla 2011-07-20 15:49:40 UTC
Git done (by process-git-requests).

Comment 9 Ian McLeod 2013-08-29 13:40:17 UTC
Package Change Request
======================
Package Name: imagefactory
New Branches: el6
Owners: imcleod

Comment 10 Gwyn Ciesla 2013-08-29 14:02:51 UTC
Git done (by process-git-requests).


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