Bug#796635: Systemd job

[Felipe Sateler <fsateler@debian.org>]

On 2 November 2015 at 18:57, Bryan Quigley <bryan.quigley@canonical.com> wrote:
> Thanks for the review Felipe
>
>> Why did you preserve runlevel S? I don't think this really belongs in
>> recovery mode.
> Changed
>
>>> +    ;;
>>> +  stop|restart|reload|force-reload)
>>
>> restart (and force-reload?) should probably re-run the recovery script.
>
> Changed.
>
>
>>> +Description=To recover nvi edit sessions.
>>
>> s/To r/R/ ?
> +1
>
>> Also, the init script Provides: nviboot, so you should provide a link
>> from nviboot.service to nvi.service in /lib/systemd/system.
> I changed it so it provided nvi.  That was the whole reason I changed
> the name afterall.

OK.

>
>>> ++if [ -n "$sessions_found" ] ; then
>>> ++        echo "done."
>>> ++else
>>> ++        echo "none found."
>>> ++fi
>>
>> This is a behavior change: previously the recover script would not
>> print any output. Maybe this should not be printed?
>
> The previous init script did print none found though.

Maybe I'm worrying too much (I leave it to you to decide), but maybe
people are using this in cronjobs or other scripted environments, and
this changes the output.

>
>> Why do you invoke dh_systemd_enable manually? the --with=systemd above
>> should do it.
>>
>>> +     dh_installinit -- start 70 S .
>>
>> start argument is obsolete now and just invokes `defaults`, so this
>> override can go.
>
> Because I didn't know what i was doing, oops.

You removed the --no-start --no-restart-on-upgrade arguments to
dh_systemd_start, are you sure you want to restart on package
install/upgrade?

>
>> However, this changes the name of the init script from nviboot to nvi.
>> This means you need to use dpkg-maintscript-helper to handle the
>> rename, and remove the old enablement links as well. If you want to do
>> this you might as well move it out of runlevel S ;)
>
> Moved out of runlevel S and it deletes nviboot correctly now.

Cool. Except for 3 things:

1. Instead of adding the rm_connfile calls maually, you can use the
dh_installdeb support. Check the manpage for details (basically, put
the rm_conffile part only once in debian/nvi.maintscript).

2. This is not really a rm_conffile, but rather a mv_conffile (the
script is renamed, not removed).

3. This will not preserve enable/disable modifications by the user.
There is no simple solution for this (you need to check in preinst the
state if differing from the default, and restore it in postinst for
the new script name), so maybe just adding a note to NEWS.Debian
suffices? Any modifications made to the init script will be lost
anyways when using systemd as then only the service file will be used.


Thanks for your work!

-- 

Saludos,
Felipe Sateler