https://bugzilla.redhat.com/show_bug.cgi?id=1445887 Petr Šabata <psabata@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+ --- Comment #5 from Petr Šabata <psabata@xxxxxxxxxx> --- (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@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
