[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #12 from Fedora Update System  ---
standard-test-roles-0.3-1.fc25 has been pushed to the Fedora 25 stable
repository. If problems still persist, please make note of it in this bug
report.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ON_QA   |CLOSED
 Resolution|--- |ERRATA
Last Closed||2017-05-08 10:21:22



--- Comment #11 from Fedora Update System  ---
standard-test-roles-0.3-1.fc26 has been pushed to the Fedora 26 stable
repository. If problems still persist, please make note of it in this bug
report.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #10 from Fedora Update System  ---
standard-test-roles-0.3-1.fc26 has been pushed to the Fedora 26 testing
repository. If problems still persist, please make note of it in this bug
report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here:
https://bodhi.fedoraproject.org/updates/FEDORA-2017-12ce8ed46e

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|MODIFIED|ON_QA



--- Comment #9 from Fedora Update System  ---
standard-test-roles-0.3-1.fc25 has been pushed to the Fedora 25 testing
repository. If problems still persist, please make note of it in this bug
report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here:
https://bodhi.fedoraproject.org/updates/FEDORA-2017-cbd26ed269

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #8 from Fedora Update System  ---
standard-test-roles-0.3-1.fc25 has been submitted as an update to Fedora 25.
https://bodhi.fedoraproject.org/updates/FEDORA-2017-cbd26ed269

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #7 from Fedora Update System  ---
standard-test-roles-0.3-1.fc26 has been submitted as an update to Fedora 26.
https://bodhi.fedoraproject.org/updates/FEDORA-2017-12ce8ed46e

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ASSIGNED|MODIFIED



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #6 from Gwyn Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/rpms/standard-test-roles

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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

Petr Šabata  changed:

   What|Removed |Added

  Flags|fedora-review?  |fedora-review+



--- Comment #5 from Petr Šabata  ---
(In reply to Merlin Mathesius from comment #4)
> Thank you for the feedback!
> 
> > * Any reason to define an additional macro, %srcname, instead of simply
> >   using %name, which happens to be identical in this case?
> 
> There is absolutely no reason in this case. It was a leftover from the spec
> file I used as a starting point. Fixed.

Ack.

> > * These roles execute quite a lot of stuff which the package doesn't 
> > require.
> >   How do you guarantee the required binaries will be present on the system?
> >   Is there a standard Ansible set you can rely on?
> 
> The first or second play in each of the role playbooks (*/tasks/main.yml) is
> a "package" task that ensures each of the packages required by the playbook
> are installed/updated.

I later realized these roles run on the tested system, not on the tester.  This
comment didn't make much sense :)

> > * Consider using install instead of mkdir and cp.
> 
> I originally tried that, but to my surprise, 'install' does not support
> recursive installation of directory trees. It would be possible to use
> 'find' in combination with 'install', but that seems cumbersome when 'cp'
> can do the job and the packaging tools automatically set proper default file
> permissions. [1] [2]
> 
> [1]
> https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_basics
> [2] https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

Ok.

> > * Missing build dependency: coreutils
> 
> Fixed.

Ack.

> > * Perhaps you could install to the new location and provide symlinks in 
> > /etc,
> >   although that somehow doesn't feel right.  It would make your package
> >   compatible with both the new and old ansible, however.  What do you think?
> 
> I agree it doesn't feel right. Fortunately, the new version of Ansible will
> continue to look for roles in /etc/ansible/roles, but will also first look
> in ~/.ansible/roles and /usr/share/ansible/roles. When the new version of
> Ansible is released, I'll change the spec to install to the new location and
> include the appropriate "Required: ansible >= W.X.Y.Z".
> 
> I also removed the "(noreplace)" option from the current %config line. The
> shared role files _should_ be replaced by package updates.

Okay, makes sense.  Thanks for the explanation.

--

I think the package is fine.  Approving.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #4 from Merlin Mathesius  ---
Thank you for the feedback!

> * Any reason to define an additional macro, %srcname, instead of simply
>   using %name, which happens to be identical in this case?

There is absolutely no reason in this case. It was a leftover from the spec
file I used as a starting point. Fixed.

> * These roles execute quite a lot of stuff which the package doesn't require.
>   How do you guarantee the required binaries will be present on the system?
>   Is there a standard Ansible set you can rely on?

The first or second play in each of the role playbooks (*/tasks/main.yml) is a
"package" task that ensures each of the packages required by the playbook are
installed/updated.

> * Consider using install instead of mkdir and cp.

I originally tried that, but to my surprise, 'install' does not support
recursive installation of directory trees. It would be possible to use 'find'
in combination with 'install', but that seems cumbersome when 'cp' can do the
job and the packaging tools automatically set proper default file permissions.
[1] [2]

[1] https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_basics
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

> * Missing build dependency: coreutils

Fixed.

> * Perhaps you could install to the new location and provide symlinks in /etc,
>   although that somehow doesn't feel right.  It would make your package
>   compatible with both the new and old ansible, however.  What do you think?

I agree it doesn't feel right. Fortunately, the new version of Ansible will
continue to look for roles in /etc/ansible/roles, but will also first look in
~/.ansible/roles and /usr/share/ansible/roles. When the new version of Ansible
is released, I'll change the spec to install to the new location and include
the appropriate "Required: ansible >= W.X.Y.Z".

I also removed the "(noreplace)" option from the current %config line. The
shared role files _should_ be replaced by package updates.


New Spec URL:
https://merlinm.fedorapeople.org/package-review/standard-test-roles/standard-test-roles.spec
New SRPM URL:
https://merlinm.fedorapeople.org/package-review/standard-test-roles/standard-test-roles-0.2-2.fc26.src.rpm
New Koji scratch build URL:
https://koji.fedoraproject.org/koji/taskinfo?taskID=19376762

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #3 from Petr Šabata  ---
* Any reason to define an additional macro, %srcname, instead of simply
  using %name, which happens to be identical in this case?

* These roles execute quite a lot of stuff which the package doesn't require.
  How do you guarantee the required binaries will be present on the system?
  Is there a standard Ansible set you can rely on?

* Consider using install instead of mkdir and cp.

* Missing build dependency: coreutils

* Perhaps you could install to the new location and provide symlinks in /etc,
  although that somehow doesn't feel right.  It would make your package
  compatible with both the new and old ansible, however.  What do you think?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #2 from Merlin Mathesius  ---
Updated to sync with new upstream release 0.2.

Spec URL:
https://merlinm.fedorapeople.org/package-review/standard-test-roles/standard-test-roles.spec
SRPM URL:
https://merlinm.fedorapeople.org/package-review/standard-test-roles/standard-test-roles-0.2-1.fc26.src.rpm
Koji scratch build URL:
https://koji.fedoraproject.org/koji/taskinfo?taskID=19272432

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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



--- Comment #1 from Merlin Mathesius  ---
Thank you for accepting the review of this package.

Please note that I am aware that /etc (specifically, /etc/ansible/roles) is not
the ideal location to deliver the shared Ansible roles, but that is where
Ansible can currently find them. This package will be updated to change the
delivery location to /usr/share/ansible/roles after Ansible PR
https://github.com/ansible/ansible/pull/23038 becomes available in Fedora.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1445887] Review Request: standard-test-roles - Standard Test Interface Ansible roles

[bugzilla]

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

Petr Šabata  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
 CC||psab...@redhat.com
   Assignee|nob...@fedoraproject.org|psab...@redhat.com
  Flags||fedora-review?



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org