Bug 1445887

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.

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

* 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?

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

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

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/standard-test-roles

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

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

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

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

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.

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.