[pmg-devel] [PATCH pmg-api 1/2] Use ucf to handle template overrides.
Stoiko Ivanov
s.ivanov at proxmox.com
Wed Apr 8 11:51:23 CEST 2020
Hi,
Thanks for the review!
On Tue, 07 Apr 2020 20:04:23 +0200
Fabian Grünbichler <f.gruenbichler at proxmox.com> wrote:
> On March 25, 2020 7:42 pm, Stoiko Ivanov wrote:
> > ucf(1) is a utility to track changes in config files which are not shipped in
> > the debian package (but e.g. get generated through the postinst script)
> >
> > While the template overriding mechanism of PMG does not directly write those
> > config files - users who override a config-file currently need to manually
> > compare the templates shipped in '/var/lib/pmg/templates' on every upgrade.
> >
> > By selectively registering the existing template overrides with ucf, users get
> > asked once upon the next upgrade regarding their changes, and then will
> > always get prompted when the shipped default template changes.
> >
> > The alternative of unconditionally registering all templates with ucf, as done
> > by dh_ucf (1), would copy all templates to /etc/pmg/templates, which I deemed
> > less elegant.
>
> The net effect would be the same, modulo that existence of the file in
> /etc now implies some manual/explicit action by the admin, and would not
> in the other variant - correct?
Yes from a functionality point of view the effect of having the templates
additionally in /etc/pmg/templates makes no difference. Only registering
already overridden templates comes from the thought, that
creating /etc/pmg/templates and actually copying and changing there is
'advanced' usage. Also it makes seeing what actually is overridden a bit
harder (e.g. we do paste all templates in /etc/pmg/templates into a
`pmg-system-report`, which does help to get a quick overview of changes)
>
> Both sound okay to me.
>
> >
> > The changes in the maintainer scripts are based on the generated ones from
> > dh_ucf (1), with the replacement of `which ucf` by the (more portable)
> > `command -v ucf` in the postrm script.
> >
> > Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> > ---
> > debian/postinst | 20 +++++++++++++++++++-
> > debian/postrm | 26 ++++++++++++++++++++++++++
> > src/Makefile | 1 +
> > 3 files changed, 46 insertions(+), 1 deletion(-)
> > create mode 100644 debian/postrm
> >
> > diff --git a/debian/postinst b/debian/postinst
> > index 5e4db3a..9118dff 100644
> > --- a/debian/postinst
> > +++ b/debian/postinst
> > @@ -3,6 +3,17 @@
> > set -e
> >
> >
> > +ucf_register_templates() {
> > + PMG_TEMPLATES=""
>
> it's very non-obvious that this is a placeholder that gets set via make.
> we usually add '.in' to the filename, and use some placeholder syntax
> like
>
> PMG_TEMPLATES="@PMG_TEMPLATE_FILES@"
did not remember/see the option of having a '.in' file - and wanted to
keep the script syntactically correct - but you're definitely right -
would have been better ...
>
> > +
> > + for template in ${PMG_TEMPLATES}; do
> > + if [ -e "/etc/pmg/templates/${template}" ]; then
> > + ucf "/var/lib/pmg/templates/${template}" "/etc/pmg/templates/${template}"
> > + ucfr pmg-api "/etc/pmg/templates/${template}"
> > + fi
> > + done
>
> but I am not sure we need that here - couldn't we just
>
> for template in /etc/pmg/templates/*; do
> if [ -e "/var/lib/pmg/templates/$(basename $template)" ]; then
> ... ucf thing ...
> else
> echo "template '$(basename $template)' no longer shipped by pmg-api, consider removing it.."
> ucf unregister/purge ... (possibly? not sure :-P whether that makes sense or not)
> fi
> done
>
> it's even less work in the default case :-P the semantics are not quite
> the same though, so maybe I am missing something.
Seems promising - I'll rewrite it accordingly for the v2 and test with the
corner case of someone putting a unrelated file there (/etc/pmg/templates
is one directory, which is synchronized at a fixed path in a cluster - so
I guess some users might have used it for unrelated custom configuration.
On the first glance it should work out nicely though!
>
> > +}
> > +
> > case "$1" in
> > triggered)
> >
> > @@ -24,6 +35,8 @@ case "$1" in
> >
> > pmgconfig init
> > pmgdb init
> > +
> > + ucf_register_templates
> > pmgconfig sync --restart
> >
> > if [ -z "$2" ]; then
> > @@ -36,13 +49,18 @@ case "$1" in
> > pmgdb update >/dev/null 2>&1 &
> >
> > update-mime-database /usr/share/mime
> > +
> > else
> > # rewrite banner
> > pmgbanner || true
> > fi
> > ;;
> >
> > - abort-upgrade|abort-remove|abort-deconfigure)
> > + abort-upgrade)
> > + ucf_register_templates
> > + ;;
> > +
> > + abort-remove|abort-deconfigure)
> > ;;
> >
> > *)
> > diff --git a/debian/postrm b/debian/postrm
> > new file mode 100644
> > index 0000000..1c14ce7
> > --- /dev/null
> > +++ b/debian/postrm
> > @@ -0,0 +1,26 @@
> > +#!/bin/sh
> > +
> > +set -e
> > +
> > +if [ "$1" = "purge" ]; then
> > + PMG_TEMPLATES=""
>
> same here, and
>
> > +
> > + for template in ${PMG_TEMPLATES}; do
> > + for ext in .ucf-new .ucf-old .ucf-dist ""; do
> > + rm -f "/etc/pmg/templates/${template}${ext}"
>
> here as well - these could just be globbed/'find .. -delete'd? we want
> to remove them, no matter whether they are for templates which were
> shipped by the last installed version?
probably a corner case (since we're purging the package here) - but the
postrm should not remove files that were put there by the local admin -
but we could iterate over the shipped ones in the prerm (or probably only
remove the registered ones)
>
> > + done
> > + if command -v ucf >/dev/null 2>&1; then
> > + ucf --purge "/etc/pmg/templates/${template}"
> > + fi
> > + if command -v ucfr >/dev/null 2>&1; then
> > + ucfr --purge pmg-api "/etc/pmg/templates/${template}"
>
> according to "man 1 ucrf", the recommended way to do this is (with a
> slight fixup, since the example does not purge :-P)
Thanks for catching this - should have not only relied on dh doing the
right thing ;)
>
> ucfq -w pmg-api | cut -d : -f 1 | while read cfile ; do ucfr --purge $cfile ; done
>
> and since all templates in /etc that ucf knows about should be
> registered, the loop can be extended to call 'ucf --purge' as well, and
> the postrm script does not need to hardcode the template file names.
>
>
> > + fi
> > + done
> > +fi
> > +
> > +# dh_installdeb will replace this with shell code automatically
> > +# generated by other debhelper scripts.
> > +
> > +#DEBHELPER#
> > +
> > +
> > diff --git a/src/Makefile b/src/Makefile
> > index b5e4c9f..3617f05 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -192,6 +192,7 @@ install: ${SOURCES} $(addsuffix .service-bash-completion, ${SERVICES}) $(addsuff
> > install -d -m 0755 ${DESTDIR}/lib/systemd/system
> > for i in ${TIMER_UNITS}; do install -m 0644 $$i ${DESTDIR}/lib/systemd/system/; done
> > install -D -m 0644 pmg-initramfs.conf ${DESTDIR}/etc/initramfs-tools/conf.d/pmg-initramfs.conf
> > + sed -ri 's/^ PMG_TEMPLATES=.*$$/PMG_TEMPLATES="${TEMPLATES}"/' debian/postinst debian/postrm
>
> this would then not be needed anymore AFAICT ?
>
> >
> > .PHONY: check
> > check:
> > --
> > 2.20.1
> >
> >
> > _______________________________________________
> > pmg-devel mailing list
> > pmg-devel at pve.proxmox.com
> > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> >
> >
More information about the pmg-devel
mailing list