https://bugzilla.redhat.com/show_bug.cgi?id=1670656 --- Comment #4 from Mark Goodwin <mgoodwin@xxxxxxxxxx> --- (In reply to Xavier Bachelot from comment #3) > Thanks for the update Mark. > Here's a couple more comments: thanks, much appreciated > > - Seems Summary is not actually fixed: > Summary: Grafana is an open source, feature rich metrics dashboard > and graph editor > s/Grafana is an open source, feature rich m/M/ oh, I thought you ment in this BZ summary (Doh!). I'll fix it in the spec too. > > - When installing files, use install -p to preserve timestamps. > > - Some of the %attr are useless in %files. All of them that force the > owner/group to root/root are redundant with the default %defattr and those > that also force the perms are also redundant with the install calls. ok > Remove the %attr in the following lines: > %attr(0755, root, root) %{_sbindir}/%{name}-server > %attr(0755, root, root) %{_sbindir}/%{name}-cli > %config(noreplace) %attr(-, root, root) > %{_sysconfdir}/sysconfig/grafana-server > %config(noreplace) %attr(-, root, root) %{_unitdir}/grafana-server.service > %attr(-, root, root) %doc %{_datadir}/doc/%{name}/*.md > %attr(-, root, root) %doc %{_datadir}/doc/%{name}/VERSION ok will do > > - The systemd unit file should not be %config(noreplace). Sorry if I did > make change you that the other way with my earlier comment. > https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/#_packaging will fix that up > > - Rather than use an %exclude in %files, rm the unneeded docs dir in %install > %exclude %{_datadir}/%{name}/docs ok. Might also consider a subpackage for grafana-docs. And will need a man page for grafana-server(1) and grafana-cli(1) > > - %files is missing a %license. The license file might be installed > elsewhere, I haven't looked, but it should be marked as such. ok will investigate that and add as appropriate > > (In reply to Mark Goodwin from comment #2) > > > > > > %post could probably use some systemd macros to be made simpler. > > > https://fedoraproject.org/wiki/Packaging:Scriptlets#Scriptlets > > > > > > Generally speaking, %post and %posttrans looks quite complicated and doing too much. Maybe that could be made simpler ? > > > > > > > these scriptlets are actually taken directly from grafana source > > See packaging/rpm/control/{postinst,posttrans}. So I'd need to > > discuss upstream before changing them. > > > I understand and if the Fedora review can benefit upstream package too, I'm > sure none would complain :-) really, all %post should need is: systemctl condrestart grafana-server (so it's restarted ONLY if it's already running). And the user/group creation. However, since current grafana.com RPMs have the config DB in /usr/share/grafana/data we might want to migrate that to it's new location in /var/lib/grafana/data or this could be left for a sysadmin to tackle. > > Anyway, let's start looking at what's in there. I think that's the part of > the specfile that need the more work. yep > > It's not silent, and scriptlets are supposed to be. > It checks for files that the rpm deploys and override them with saved config > files, which voids the purpose of %config(noreplace). yep agree - shouldn't be necessary with correct config(noreplace) > It changes owner/group and perms on files that already have been properly > set in the rpm. agree, shouldn't need to do that either > I might be missing something, but it seems part useless and part dangerous > to me. > > I won't detail %post for now, but it seems to suffer from the same > symptoms(set owner/group/perms, install conf file that are/should be > deployed by the rpm). > Enabling services on rpm install is also probably wrong. against fedora and rhel policy > Again, unless I'm missing something, the only parts that should be kept are > the grafana user and group creation and the conditional restart of the > service on upgrade. yep - I'll work through that and post a new update later today (I'm in Australia TZ) > https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/ > https://docs.fedoraproject.org/en-US/packaging-guidelines/DefaultServices/ > https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/ > #_systemd Cheers -- 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 Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
