[Bug 1324204] New: Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

Bug ID: 1324204
           Summary: Review Request: fast-vm - script for defining VMs from
                    images provided in thin LVM pool.
           Product: Fedora
           Version: 22
         Component: Package Review
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
            Blocks: 177841 (FE-NEEDSPONSOR)

Spec URL:
https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm-fedo...
SRPM URL:
https://github.com/OndrejHome/fast-vm/releases/download/0.4/fast-vm-0.4-22.f...
Description: fast-vm is a script for defining VMs from images provided in thin
LVM pool.
fast-vm is taking care of:
- defining the VMs from provided XML in libvirt
- creating thin LV thin snaphost as storage devices for VMs
- making static IP DHCP reservation in libvirt network definition for MAC
address of VM

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #1 from Ondrej Faměra  ---
Currently I'm working toward next release (0.5) and below is snapshot from the
most recent commit c287b1e1a23c0a9e3f874c04f1d0954b56cf367d.

Spec URL: https://ssl.famera.cz/tmp/fast-vm-fedora22.spec
SRPM URL: https://ssl.famera.cz/tmp/fast-vm-0.4-1.fc22.src.rpm

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

Petr Šabata  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review?

--- Comment #2 from Petr Šabata  ---
I'll take a look and possibly sponsor Ondrej.

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #3 from Petr Šabata  ---
What is your FAS username?  You didn't include it in the request.

As for the review, I'll just start from the top.

* Your package is noarch, therefore defining the first three macros is fairly
  pointless.  You're not getting any debuginfo for your shell script anyway.
  You can drop these.

* A cosmetic detail: consider expanding the tabs and aligning all the values.

* Another cosmetic detail: remove all trailing whitespace.

* Summary should begin with a capital letter.

* It's unclear whether the license is really `GPLv3' or `GPLv3+'.  Since you
  are also the upstream -- if you're sure you really want `GPLv3', consider
  noting that in the README file.

* The source tarball included in the SRPM differs from the one available on
  GitHub (!).

* A split (and ordered) list of runtime dependencies would be more readable.
  For example:
    Requires:  curl
    Requires:  dnsmasq-utils
    ...

* I'm not going to comment your your runtime dependencies yet.  Provide an
  updated package with sources available upstream first.

* Your package has no %changelog entries.  Add one. "Initial release" would do.

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #4 from Ondrej Faměra  ---
FAS username: ondrejhome

SRPM URL:
https://github.com/OndrejHome/fast-vm/releases/download/0.5/fast-vm-0.5-1.fc...
Spec URL:
https://raw.githubusercontent.com/OndrejHome/fast-vm/develop/rpm/fast-vm-fed...

- removed unneeded macro in the start
- alignments and white spaces hopefully fixed
- summary corrected
- license changed to GPLv3+
- source tarball and SRPM is now from exactly same source (GitHub)
- list of dependencies split and ordered, additionally changed some
dependencies to recommends only
- added initial changelog entry

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #5 from Ondrej Faměra  ---
new version of fast-vm (0.6)

SRPM URL:
https://github.com/OndrejHome/fast-vm/releases/download/0.6/fast-vm-0.6-4.fc...
Spec URL:
https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm-fedo...

- no major changes in RPM .spec file since 0.5 (just added changelog entry)

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #6 from Petr Šabata  ---
Okay, let's take a look.  Sorry for the delay.

(In reply to Petr Šabata from comment #3)
> Your package is noarch, therefore defining the first three macros is fairly
> pointless.  You're not getting any debuginfo for your shell script anyway.
> You can drop these.

Removed.  Ack.
> A cosmetic detail: consider expanding the tabs and aligning all the values.

You still use tabs but it looks reasonable with tabwidth=8.
> Another cosmetic detail: remove all trailing whitespace.

Removed.  Ack.
> Summary should begin with a capital letter.

Corrected.  Ack.
> It's unclear whether the license is really `GPLv3' or `GPLv3+'.  Since you
> are also the upstream -- if you're sure you really want `GPLv3', consider
> noting that in the README file.

Changed to GPL3+ and clarified upstream.  Ack.
> The source tarball included in the SRPM differs from the one available on
> GitHub (!).

The files are the same now.  Ack.
> A split (and ordered) list of runtime dependencies would be more readable.
> For example:
>   Requires:  curl
>   Requires:  dnsmasq-utils
>   ...

It looks reasonable now.  I have yet to verify whether it's actually correct.
> I'm not going to comment your your runtime dependencies yet.  Provide an
> updated package with sources available upstream first.
>
> Your package has no %changelog entries.  Add one. "Initial release" would
>
> do.

Changelog is fine.  Ack.

New comments:

* Your SPEC file name doesn't match the package name.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_File_Naming

* %description line 29 is too long.
  Line length shouldn't exceed 80 characters; cut it into two.

* %{_libexecdir}/%{name}-helper.sh permissions look strange.
  The file isn't world readable/executable.  Wouldn't 755 make more sense?

* It appears 440 permissions are more common for sudoers files.
  This isn't very important but consider changing yours.

* Mark your sudoers file as configuration.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files

* You should own the %{_datadir}/bash-completion/completions directory.
  Just change line 45 to %{_datadir}/bash-completion/completions.

I'll review your dependencies tomorrow, hopefully.

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #7 from Ondrej Faměra  ---
> Your SPEC file name doesn't match the package name.
> https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_File_Naming

- renamed to fast-vm.spec
> %description line 29 is too long.
> Line length shouldn't exceed 80 characters; cut it into two.

- done, put into new line
> %{_libexecdir}/%{name}-helper.sh permissions look strange.
>
> The file isn't world readable/executable.  Wouldn't 755 make more sense?

- the file is not intended to be executable by anyone other than root, but can
be readable by people - changed to 744.
> It appears 440 permissions are more common for sudoers files.
>
> This isn't very important but consider changing yours.

- changed to 440
> Mark your sudoers file as configuration.
> https://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files

- marked
> You should own the %{_datadir}/bash-completion/completions directory.
> Just change line 45 to %{_datadir}/bash-completion/completions.

- changed

Thank you for suggestions. New .spec file and SRPM with changes from above can
be found below.

SRPM URL:
https://github.com/OndrejHome/fast-vm/releases/download/0.6.1/fast-vm-0.6.1-...
Spec URL:
https://raw.githubusercontent.com/OndrejHome/fast-vm/develop/rpm/fast-vm.spe...

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #8 from Petr Šabata  ---
(In reply to Ondrej Faměra from comment #7)
> > Your SPEC file name doesn't match the package name.
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_File_Naming
> renamed to fast-vm.spec

Ack.
> > %description line 29 is too long.
> > Line length shouldn't exceed 80 characters; cut it into two.
> done, put into new line

Ack.
> > %{_libexecdir}/%{name}-helper.sh permissions look strange.
> >
> > The file isn't world readable/executable.  Wouldn't 755 make more sense?
> the file is not intended to be executable by anyone other than root, but
>
> can be readable by people - changed to 744.

Alright.
> > It appears 440 permissions are more common for sudoers files.
> >
> > This isn't very important but consider changing yours.
> changed to 440

Ack.
> > Mark your sudoers file as configuration.
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files
> marked

Ack.
> > You should own the %{_datadir}/bash-completion/completions directory.
> > Just change line 45 to %{_datadir}/bash-completion/completions.
> changed

Ack.
> Thank you for suggestions. New .spec file and SRPM with changes from above
> can be found below.
>
> SRPM URL:
> https://github.com/OndrejHome/fast-vm/releases/download/0.6.1/fast-vm-0.6.1-
> 1.fc22.src.rpm
> Spec URL:
> https://raw.githubusercontent.com/OndrejHome/fast-vm/develop/rpm/fast-vm.spe...

--

Now for the dependencies -- we've gone through them together in person so I'll
just list the required changes in here without explaining that in detail:

- Don't require bash
- Require coreutils
- Require ncurses
- Require openssh-clients
- Require sed
- dnsmasq-utils should be required, not recommended
- Recommend awk
- Remove the libguestfs-tools-ca "suggests"

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #9 from Ondrej Faměra  ---
> Don't require bash
> Remove the libguestfs-tools-ca "suggests"

- removed
> Require coreutils
> Require ncurses
> Require openssh-clients
> Require sed

- added
> dnsmasq-utils should be required, not recommended

- updated the code in the fast-vm-helper.sh so the dnsmasq-utils can be only
recommended
> Recommend awk

- added as 'required' as the new listing functions ('fast-vm list') requires
this

Updated .spec file and SRPM below

SRPM URL:
https://github.com/OndrejHome/fast-vm/releases/download/0.7/fast-vm-0.7-1.fc...
Spec URL:
https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm.spec

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

--- Comment #10 from Ondrej Faměra  ---
Hotfix 0.7.1

- wrong 'awk' requirement, changed to 'gawk'
- updated the description of the package to match the description
from the README

SRPM URL:
https://github.com/OndrejHome/fast-vm/releases/download/0.7.1/fast-vm-0.7.1-...
Spec URL:
https://raw.githubusercontent.com/OndrejHome/fast-vm/master/rpm/fast-vm.spec

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

Petr Šabata  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|177841 (FE-NEEDSPONSOR)     |

--- Comment #11 from Petr Šabata  ---
Acking all the changes.
The package builds and install fine.

I'm tentatively approving the review.

However, before I really do that and sponsor you into the packaging group, I'd
like to see you prepare another package.  It doesn't have to be anything huge. 
Just to see you really begin to understand the whole packaging thing.

If you're out of ideas, check out the package wishlist:
https://fedoraproject.org/wiki/Package_maintainers_wishlist?rd=PackageMainta...

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor

[Bug 1324204] Review Request: fast-vm - script for defining VMs from images provided in thin LVM pool.

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1324204

Fedora End Of Life  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|---                         |EOL
        Last Closed|                            |2016-07-19 14:47:42

--- Comment #12 from Fedora End Of Life  ---
Fedora 22 changed to end-of-life (EOL) status on 2016-07-19. Fedora 22 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.