On 2 Nov 2015 18:58, "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.
>
> >> ++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.
>
> > 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.
>
> > 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.
>
Also, you might want to remove the - from the su call. A login shell is not required.
Saludos