[Merge] lp:~seb128/update-notifier/systemd into lp:update-notifier

Martin Pitt martin.pitt at ubuntu.com
Wed Jul 20 16:34:47 UTC 2016


Review: Needs Fixing

Looks mostly good, thanks for working on this! I found some nitpicks.

Diff comments:

> 
> === added file 'debian/systemd/unicast-local-avahi.path'
> --- debian/systemd/unicast-local-avahi.path	1970-01-01 00:00:00 +0000
> +++ debian/systemd/unicast-local-avahi.path	2016-07-20 16:02:02 +0000
> @@ -0,0 +1,6 @@
> +[Unit]
> +Description=Notification regarding avahi disabled due to .local domain

Nitpick: This should be different than the .service description, easier to tell apart in the logs for debugging. "Path trigger for Avahi .local domain notifications"?

> +PartOf=graphical-session.target
> +
> +[Path]
> +PathChanged=/var/run/avahi-daemon/disabled-for-unicast-local

Just /run please, no new instances of /var/run. For gold stars, change all other occurrences of /var/run to /run in update-notifier. :-)

> 
> === added file 'debian/systemd/update-notifier-crash.path'
> --- debian/systemd/update-notifier-crash.path	1970-01-01 00:00:00 +0000
> +++ debian/systemd/update-notifier-crash.path	2016-07-20 16:02:02 +0000
> @@ -0,0 +1,6 @@
> +[Unit]
> +Description=Notification regarding a crash report

Dito, "Path trigger for Apport crash notifications" or so?

> +PartOf=graphical-session.target
> +
> +[Path]
> +PathChanged=/var/crash/
> 
> === added file 'debian/systemd/update-notifier-crash.service'
> --- debian/systemd/update-notifier-crash.service	1970-01-01 00:00:00 +0000
> +++ debian/systemd/update-notifier-crash.service	2016-07-20 16:02:02 +0000
> @@ -0,0 +1,6 @@
> +[Unit]
> +Description=Notification regarding a crash report

Add PartOf=graphical-session.target please, this is potentially a long-running service and it should cleanly stop with logging out of the session.

> +
> +[Service]
> +ExecStart=/usr/lib/update-notifier/update-notifier-crash
> +Type=oneshot
> 
> === added file 'debian/systemd/update-notifier-release.path'
> --- debian/systemd/update-notifier-release.path	1970-01-01 00:00:00 +0000
> +++ debian/systemd/update-notifier-release.path	2016-07-20 16:02:02 +0000
> @@ -0,0 +1,7 @@
> +[Unit]
> +Description=Notification regarding a new release of Ubuntu

"Path trigger.."

> +PartOf=graphical-session.target
> +
> +[Path]
> +PathChanged=/var/lib/ubuntu-release-upgrader/release-upgrade-available
> +

Trailing empty line

> 
> === added file 'debian/systemd/update-notifier-release.service'
> --- debian/systemd/update-notifier-release.service	1970-01-01 00:00:00 +0000
> +++ debian/systemd/update-notifier-release.service	2016-07-20 16:02:02 +0000
> @@ -0,0 +1,6 @@
> +[Unit]
> +Description=Notification regarding a new release of Ubuntu

PartOf=g-s.t

> +
> +[Service]
> +ExecStart=/usr/lib/ubuntu-release-upgrader/check-new-release-gtk
> +Type=oneshot
> 
> === modified file 'debian/update-notifier-cds.conf'
> --- debian/update-notifier-cds.conf	2015-01-20 09:34:45 +0000
> +++ debian/update-notifier-cds.conf	2016-07-20 16:02:02 +0000
> @@ -13,7 +13,7 @@
>  DATE=$(date)
>  echo "$DATE CD found at $DEVNAME"
>  sleep 10
> -MOUNTPOINT=$(printf $(awk "{if (\$1 == \"$D\") {print \$2}}" /proc/mounts ))
> +MOUNTPOINT=$(printf $(awk "{if (\$1 == \"$DEVNAME\") {print \$2}}" /proc/mounts ))

*brown paperbag* thanks for cleaning up after me!

>  echo "CD mounted at $MOUNTPOINT"
>  if [ -L "$MOUNTPOINT/ubuntu" ] || [ -e "$MOUNTPOINT/cdromupgrade" ]
>  then


-- 
https://code.launchpad.net/~seb128/update-notifier/systemd/+merge/300634
Your team Ubuntu Core Development Team is requested to review the proposed merge of lp:~seb128/update-notifier/systemd into lp:update-notifier.



More information about the Ubuntu-reviews mailing list