[Bug 638647] Review Request: mom - Dynamically manage system resources on virtualization hosts

[bugzilla at redhat.com]

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=638647

--- Comment #6 from Adam Litke <agl at us.ibm.com> 2010-10-26 13:01:15 EDT ---
Thank you for your review.  The latest files can be found below:

http://github.com/downloads/aglitke/mom/mom.spec
http://github.com/downloads/aglitke/mom/mom-0.2.1-3.fc13.src.rpm

In reply to comment #5)
> I'm not a sponsor, so I won't be able to approve the package, but here
> are a couple more comments.
> 
> - I'd rather see the patch split into 2 patches, one for the initscript
> and one for the spec. Actually, the spec part is useless, as the bundled
> spec file is not used.

Done.  I want to keep the spec file patch so I can track the changes that need
to be committed to the MOM tree once this review process is complete.

> - The initscript part of the patch turns 2 lines into 1 which then will
> span over 80 characters, please fix.

Done.

> - Patches need to have an upstream reference (bug/commit/...).This is
> intended to help keep track of the patches. As you're upstream and this
> will probably make it in the leext release, this is not a big deal, but
> I thought I'd mention it.

Thanks.  These patches are a convenience for me so I don't have to keep
changing upstream for each iteration of this review.

> - The %build and %install section are not following the template. The build
> part should not be done from within the %install section. See
> /etc/rpmdevtools/spectemplate-python.spec.

Fixed.

> - The install of the initscript could be made shorter :
> install -Dp contrib/momd.init $RPM_BUILD_ROOT/%_initddir/momd

Ok.  Thanks.

> - No need to copy the LICENSE and README files to the docdir, just using
> %doc LICENSE README in the %files section will be enough.

Got it.

> - The INSTALL file doesn't contain any useful content, it's not needed
> to include it currently. This might be changed later if/when its content
> evolves.

Are you referring to the README file?  I was hoping I could leave it included
since I will be updating it soon.  I don't like the idea of simply deleting an
installed file.

> - Use macros everywhere possible. When moving the examples, replace
> /usr/share/doc by %{_defaultdocdir}.

This should be consistent now.

> - It might be better to alter setup.py to install the doc to the proper
> location rather than move them after install.

Will look into this for a future upstream release.

> - BuildRoot and %clean are not needed anymore with recent Fedora
> releases. However, this is still needed for EL5, so keep it if you want
> to also have EPEL branches. In this case, you'll also want to replace
> %{_initddir} by %{_initrddir}.

I chose to go the backward-compatible route.

> - There are some useless comments in the spec (first line and in %post).

Gone now.

> - Please consider providing a default/dummy conf file and mark it as
> %config(noreplace) in the %files section.

I now duplicate one of the example config files and install it as
/etc/momd.conf (which agrees with the init script).

> - You might want to use %{version} and possibly %{name] macros in the
> Source0 tag, so you don't have to change the version in 2 places when
> upgrading to a later version.

Agreed.

-- 
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.