[Bug 721144] Review Request: imagefactory - System image generation tool
https://bugzilla.redhat.com/show_bug.cgi?id=721144 Ian McLeod imcl...@redhat.com changed: What|Removed |Added CC||imcl...@redhat.com Flags|fedora-cvs+ |fedora-cvs? --- Comment #9 from Ian McLeod imcl...@redhat.com --- Package Change Request == Package Name: imagefactory New Branches: el6 Owners: imcleod -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=1R8KCFiA2ta=cc_unsubscribe ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
https://bugzilla.redhat.com/show_bug.cgi?id=721144 --- Comment #10 from Jon Ciesla limburg...@gmail.com --- Git done (by process-git-requests). -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=1nV27ZDPLta=cc_unsubscribe ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
https://bugzilla.redhat.com/show_bug.cgi?id=721144 Jon Ciesla limburg...@gmail.com changed: What|Removed |Added Flags|fedora-cvs? |fedora-cvs+ -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=AKvpkAWxUNa=cc_unsubscribe ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 Bug 721144 depends on bug 717966, which changed state. Bug 717966 Summary: Review Request: python-psphere - vSphere SDK for Python https://bugzilla.redhat.com/show_bug.cgi?id=717966 What|Old Value |New Value Resolution||NEXTRELEASE Status|ASSIGNED|CLOSED -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 #3 from Mark McLoughlin mar...@redhat.com 2011-07-20 04:46:51 EDT --- (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 ? -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 #4 from Chris Lalancette clala...@redhat.com 2011-07-20 10:49:51 EDT --- (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 -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 Mark McLoughlin mar...@redhat.com changed: What|Removed |Added Flag|fedora-review? |fedora-review+ --- Comment #5 from Mark McLoughlin mar...@redhat.com 2011-07-20 10:56:31 EDT --- Okay, looks good to me -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 #6 from Chris Lalancette clala...@redhat.com 2011-07-20 11:13:02 EDT --- Thanks! -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 Chris Lalancette clala...@redhat.com changed: What|Removed |Added Flag||fedora-cvs? --- Comment #7 from Chris Lalancette clala...@redhat.com 2011-07-20 11:13:57 EDT --- New Package SCM Request === Package Name: imagefactory Short Description: System image generation tool Owners: clalance -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 #8 from Jon Ciesla l...@jcomserv.net 2011-07-20 11:49:40 EDT --- Git done (by process-git-requests). -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 Chris Lalancette clala...@redhat.com changed: What|Removed |Added Status|ASSIGNED|CLOSED Resolution||RAWHIDE Last Closed||2011-07-20 13:47:53 -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 clala...@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.
- 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 21
- /sbin/service %{name} stop /dev/null 21
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@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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
Mark McLoughlin mar...@redhat.com changed:
What|Removed |Added
Status|NEW |ASSIGNED
CC||mar...@redhat.com
AssignedTo|nob...@fedoraproject.org|mar...@redhat.com
Flag||fedora-review?
--- Comment #1 from Mark McLoughlin mar...@redhat.com 2011-07-18 04:47:59 EDT
---
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 21
- /sbin/service %{name} stop /dev/null 21
- 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
--
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@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 721144] Review Request: imagefactory - System image generation tool
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 Chris Lalancette clala...@redhat.com changed: What|Removed |Added Depends on||717966 -- 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@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
