https://bugzilla.redhat.com/show_bug.cgi?id=1670656 --- Comment #3 from Xavier Bachelot <xavier@xxxxxxxxxxxx> --- Thanks for the update Mark. Here's a couple more comments: - 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/ - 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. 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 - 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 - Rather than use an %exclude in %files, rm the unneeded docs dir in %install %exclude %{_datadir}/%{name}/docs - %files is missing a %license. The license file might be installed elsewhere, I haven't looked, but it should be marked as such. (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 :-) Anyway, let's start looking at what's in there. I think that's the part of the specfile that need the more work. %posttrans #!/bin/sh set -e echo "POSTTRANS: Running script" [ -f /etc/sysconfig/grafana-server ] && . /etc/sysconfig/grafana-server # copy config files if missing if [ ! -f /etc/grafana/grafana.ini ]; then echo "POSTTRANS: Config file not found" if [ -f /etc/grafana/grafana.ini.rpmsave ]; then echo "POSTTRANS: /etc/grafana/grafana.ini.rpmsave config file found." mv /etc/grafana/grafana.ini.rpmsave /etc/grafana/grafana.ini echo "POSTTRANS: /etc/grafana/grafana.ini restored" if [ -f /etc/grafana/ldap.toml.rpmsave ]; then echo "POSTTRANS: /etc/grafana/ldap.toml.rpmsave found" mv /etc/grafana/ldap.toml.rpmsave /etc/grafana/ldap.toml echo "POSTTRANS: /etc/grafana/ldap.toml restored" fi echo "POSTTRANS: Restoring config file permissions" chown -Rh root:$GRAFANA_GROUP /etc/grafana/* chmod 755 /etc/grafana find /etc/grafana -type f -exec chmod 640 {} ';' find /etc/grafana -type d -exec chmod 755 {} ';' fi chown $GRAFANA_USER /usr/share/grafana/data chmod 755 /usr/share/grafana/data fi 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). It changes owner/group and perms on files that already have been properly set in the rpm. 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. 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. 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 -- 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
