Merge ~ahasenack/ubuntu/+source/base-files:groovy-motd-news-config-split into ubuntu/+source/base-files:ubuntu/devel

[Andreas Hasenack]

PPA via bileto: https://bileto.ubuntu.com/#/ticket/4167

This bileto run is a bit buggy, probably because of the NEw package. I'm pinging sil2100, but he is busy today with a cloud-init/grub fire.

One of 3 packages needed to fix bug https://bugs.launchpad.net/ubuntu/+source/base-files/+bug/1888575

The overall plan is to have motd-news only enabled on server systems after this upgrade. What it does:
- the /etc/default/motd-news config file is moved from base-files to a NEW package motd-news-config. If it has modifications, they are preserved
- ubuntu-server has a dependency on motd-news-config
- new base-files breaks old ubuntu-server, forcing an upgrade if it is installed

The versions used in these breaks/replaces have to be very precise for this to work, so between now and whenever these are uploaded, I'll check again.

For now in groovy we have:
base-files 11ubuntu10
ubuntu-meta 1.452

Therefore:
ubuntu-server 1.453:
Depends: motd-news-config

base-files 11ubuntu11:
Breaks/Replaces: ubuntu-server (<< 1.453)
rm_conffile /etc/default/motd-news 11ubuntu11~ base-files

motd-news-config 1 (NEW):
Breaks/Replaces: base-files (<< 11ubuntu11)

To review motd-news-config, grab the source from the ppa. It's also in a branch, but a bit messy with lots of ppaN and changelog commits.

There is one lintian error in motd-news-config, and that's about the usage of ENABLED in an /etc/default/* file. This package does not even have an initscript or systemd service: that exists in base-files. And even there it's a timer that runs the one-shot service script. I thought about adding an override, but thought it best to keep the lintian error visible for the moment. Considering this package will be SRUed all the way back to xenial systems, I don't think it warrants extensive changes to get rid of ENABLED=1/0 in that file. But the AA will have the final sya.

For the PPA, I commited directly to src:ubuntu-meta instead of running its ./update script, as I couldn't convince that script to use a branch of mine instead of the official ubuntu seeds repository. I'll propose that branch as well, but I can't land it until motd-news-config is accepted from the NEW queue (I haven't uploaded it yet).

Here are the tests I've been running:
a) base-files installed, ubuntu-server installed, unmodified /e/d/motd-news
apt install base-files
- upgrades ubuntu-server
- installs motd-news-config
- /e/d/motd-news remains, motd-news remains enabled

b) base-files installed, ubuntu-server installed, modified /e/d/motd-news
apt install base-files
- upgrades ubuntu-server
- installs motd-news-config
- /e/d/motd-news remains with the original modification

c) base-files installed, ubuntu-server not installed, unmodified /e/d/motd-news
apt install base-files
- upgrades base-files
- removes /e/d/motd-news
- motd-news is disabled

d) base-files installed, ubuntu-server not installed, modified /e/d/motd-news
apt install base-files
- upgrades base-files
- /e/d/motd-news gets renamed to backup
- motd-news is disabled

e) removing motd-news-config will also remove ubuntu-server (since it's a depends, and not a recommends)

[Andreas Hasenack (ahasenack)]

ubuntu-meta seed change: https://code.launchpad.net/~ahasenack/ubuntu-seeds/+git/ubuntu-seeds/+merge/388403

[Andreas Hasenack (ahasenack)]

My motd-news-config packaging branch, a bit messy in terms of commits:

https://code.launchpad.net/~ahasenack/ubuntu/+source/motd-news-config/+git/motd-news-config

[Bryce Harrington (bryce)]

* Changelog:
  - [√] old content and logical tag match as expected
  - [ ] changelog entry correct version and targeted codename
  - [√] update-maintainer has been run

* Actual changes:
  - [-] no upstream changes to consider
  - [-] no further upstream version to consider
  - [√] debian changes look safe

* Old Delta:
  - [-] dropped changes are ok to be dropped
  - [-] nothing else to drop
  - [?] changes forwarded upstream/debian (if appropriate)

* New Delta:
  - [-] no new patches added
  - [-] patches match what was proposed upstream
  - [-] patches correctly included in debian/patches/series
  - [-] patches have correct DEP3 metadata

* Build/Test:
  - [√] build is ok
  - [-] verified PPA package installs/uninstalls
  - [-] autopkgtest against the PPA package passes
  - [√] sanity checks test fine

The changelog entry wording sounds a little mysterious, can I suggest a small wording improvement?

Is:
  "The /etc/default/motd-news conffile is in another src package"

S/b:
  "Split the /etc/default/motd-news conffile to a separate src package, motd-news-config. Force the system to upgrade to ubuntu-server 1.453 at least when this is installed to ensure the conffile remains present."

I know Debian doesn't use our motd-news stuff, but are there any bits of this that might be applicable to Debian? I'm assuming not.

Your testing process looks sound, I didn't run those in detail however I verified the branch builds, and that attempting to install the built deb correctly refuses to do so without the updated ubuntu-server:

triage-groovy+20.10:~/pkg/BaseFiles$ sudo dpkg -i base-files_11ubuntu11_amd64.deb
dpkg: regarding base-files_11ubuntu11_amd64.deb containing base-files:
 base-files breaks ubuntu-server (<< 1.453)
  ubuntu-server (version 1.451) is present and installed.

dpkg: error processing archive base-files_11ubuntu11_amd64.deb (--install):
 installing base-files would break ubuntu-server, and
 deconfiguration is not permitted (--auto-deconfigure might help)
Errors were encountered while processing:
 base-files_11ubuntu11_amd64.deb

Other than the changelog entry everything LGTM, so I'm marking this approved and you can adjust the changelog before landing.

review: Approve

[Andreas Hasenack (ahasenack)]

> The changelog entry wording sounds a little mysterious, can I suggest a small
> wording improvement?

Thanks for the suggestion. I wanted to avoid telling the whole story in d/changelog, and leave the details in the bug, but made it too mysterious indeed :)

I changed the wording in two places, see if that's better please.

[Andreas Hasenack (ahasenack)]

> I know Debian doesn't use our motd-news stuff, but are there any bits of this that
> might be applicable to Debian? I'm assuming not.

Not a battle I'm ready to pick :(

[Bryce Harrington (bryce)]

I also did a review on the new package motd-news-config. A few notes:

* debian/copyright

This appears to still have template content needing filled out. I don't think the copyright file needs much detail; this is a pretty minimal package.

* debian/motd-news-config.motd-news.default

The grammar of the comments in this file is rather poor. Given that we're making a new package, this seems like a good opportunity to improve the user documentation text. Here's my attempt:

# Enable/disable the dynamic MOTD news service.
#
# This service provides information relevant to the local
# system administrator such as availability of upgrades,
# system status data, and similar.
#
ENABLED=1

# Source service for dynamic MOTD news.
#
# MOTD news is provided by Canonical via motd.ubuntu.com. This
# configuration parameter allows pulling it from one or more
# alternative locations (whitespace-separated). For security
# reasons, these must use the https protocol and have a valid
# certificate.
#
URLS="https://motd.ubuntu.com"

# Delay (in seconds) between MOTD fetch attempts.
#
# Note that news messages are fetched in the background by
# a systemd timer and will not block boot or login.
#
WAIT=5

I'm kind of curious about the WAIT parameter, am I interpreting this correctly that it polls every 5 seconds? That seems far too frequently so I think I'm missing something here.

I also installed base-files from the PPA, and it correctly pulled in the new package and newer ubuntu-server. It appears if you delete (or don't have) 50-motd-news, it'll prompt you to create it but default to omitting it, which seems to be appropriate behavior:

Preparing to unpack .../base-files_11ubuntu11_amd64.deb ...
Warning: Stopping motd-news.service, but it can still be activated by:
  motd-news.timer
Unpacking base-files (11ubuntu11) over (11ubuntu7) ...
Setting up base-files (11ubuntu11) ...

Configuration file '/etc/update-motd.d/50-motd-news'
 ==> Deleted (by you or by a script) since installation.
 ==> Package distributor has shipped an updated version.
   What would you like to do about it ? Your options are:
    Y or I : install the package maintainer's version
    N or O : keep your currently-installed version
      D : show the differences between the versions
      Z : start a shell to examine the situation
 The default action is to keep your current version.
*** 50-motd-news (Y/I/N/O/D/Z) [default=N] ?

review: Approve

[Andreas Hasenack (ahasenack)]

> * debian/copyright
>
> This appears to still have template content needing filled out.
> I don't think the copyright file needs much detail; this is a pretty minimal package.

Sorry, I don't see the template bits.

> * debian/motd-news-config.motd-news.default
>
> The grammar of the comments in this file is rather poor. Given that
> we're making a new package, this seems like a good opportunity to
> improve the user documentation text.

Even though it's a new package, I'm wary of changing the contents of the config file as I would like to keep the md5 hash the same as it was.

> It appears if you delete (or don't have) 50-motd-news, it'll prompt you to
> create it but default to omitting it, which seems to be appropriate behavior:

Did you mean to test /etc/default/motd-news instead?

[Andreas Hasenack (ahasenack)]

Bryce, what do you think about my comments above?

[Bryce Harrington (bryce)]

> > * debian/copyright
> >
> > This appears to still have template content needing filled out.
> > I don't think the copyright file needs much detail; this is a pretty minimal
> package.
>
> Sorry, I don't see the template bits.

Upstream-Contact: <preferred name and address to reach the upstream project>
Source: <url://example.com>

Files: *
Copyright: <years> <put author's name and email here>
           <years> <likewise for another author>
License: GPL-3.0+

I'm referring to the bits in the angle brackets

> > * debian/motd-news-config.motd-news.default
> >
> > The grammar of the comments in this file is rather poor. Given that
> > we're making a new package, this seems like a good opportunity to
> > improve the user documentation text.
>
> Even though it's a new package, I'm wary of changing the contents of the
> config file as I would like to keep the md5 hash the same as it was.

I get that, but I do think it is worthwhile to update it.

The thing that caught my eye was that the service provides "informative
information", but the grammar in the rest of the file is equally clumsy.
Fixing this will make the package look more professional. You're right
that the md5sum will change, but most people won't have modified this
config file, and groovy is LTS+1. Since we're introducing a new package
here, I don't think many people will be surprised to see the config file
updated.

> > It appears if you delete (or don't have) 50-motd-news, it'll prompt you to
> > create it but default to omitting it, which seems to be appropriate
> behavior:
>
> Did you mean to test /etc/default/motd-news instead?

No, this was just testing installation/uninstallation. Don't worry about
this, I customize my motd setup (even in my lxc containers) to make them
less chatty, so I probably don't have as relevant an environment to test.

[Andreas Hasenack (ahasenack)]

> I'm referring to the bits in the angle brackets

Sorted on irc, it was an incorrect branch.

> I get that, but I do think it is worthwhile to update it.
>
> The thing that caught my eye was that the service provides "informative
> information", but the grammar in the rest of the file is equally clumsy.
> Fixing this will make the package look more professional. You're right
> that the md5sum will change, but most people won't have modified this
> config file,

I will have to test what impact a changed md5 will have on this, both in this particular upgrade, and from that point on.

We have to be *very* careful to *never* re-enable a previously disabled motd-news config. Also note we will SRU this all the way back to xenial, and the config file I'm using is the template one since then.

[Andreas Hasenack (ahasenack)]

Thanks for the new text, I do think it looks better, but introducing that change at this time would delay these uploads even further as I would have to re-test the upgrade scenarios, with and without config file changes, all the way back to xenial. We are of course free to revisit this later, specially in groovy.

Incidentally, the WAIT parameter is for how long wget should wait at most to fetch the motd text, not how frequently it will try.

[Andreas Hasenack (ahasenack)]

First step, motd-news-config:

$ dput ubuntu ../motd-news-config_1_source.changes
Checking signature on .changes
gpg: ../motd-news-config_1_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../motd-news-config_1.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading motd-news-config_1.dsc: done.
  Uploading motd-news-config_1.tar.xz: done.
  Uploading motd-news-config_1_source.buildinfo: done.
  Uploading motd-news-config_1_source.changes: done.
Successfully uploaded packages.

82e3d94... by Andreas Hasenack on 2020-08-06

  * Move the /etc/default/motd-news conffile to the motd-news-config
    package (LP: #1888575):
    - d/base-files.maintscript: remove /etc/default/motd-news config file
      on upgrade

8265bcc... by Andreas Hasenack on 2020-08-06

    - d/motd-news-config.postinst: handle the upgrade case where the
      motd-news config file was changed while it belonged to base-files

191e383... by Andreas Hasenack on 2020-08-06

    - d/control: new motd-news-config package, carrying the
      configuration file for the /etc/update-motd.d/50-motd-news script.

b9c6368... by Andreas Hasenack on 2020-08-06

changelog

[Andreas Hasenack (ahasenack)]

Steve requested that the new motd-news-config package be part of base-files, and not a new source package on its own, so I'll abandon this MP and create a new one once I'm done with that refactoring.

Unmerged commits

b9c6368... by Andreas Hasenack on 2020-08-06

changelog

191e383... by Andreas Hasenack on 2020-08-06

    - d/control: new motd-news-config package, carrying the
      configuration file for the /etc/update-motd.d/50-motd-news script.

8265bcc... by Andreas Hasenack on 2020-08-06

    - d/motd-news-config.postinst: handle the upgrade case where the
      motd-news config file was changed while it belonged to base-files

d80daba... by Andreas Hasenack on 2020-07-28

    - d/control: break on ubuntu-server << 1.453 to force an upgrade if
      it is installed, which will pull motd-news-config and the conffile
      back in

82e3d94... by Andreas Hasenack on 2020-08-06

  * Move the /etc/default/motd-news conffile to the motd-news-config
    package (LP: #1888575):
    - d/base-files.maintscript: remove /etc/default/motd-news config file
      on upgrade

diff --git a/debian/base-files.maintscript b/debian/base-files.maintscript
 new file mode 100644
index 0000000..9972f51
--- /dev/null
+++ b/debian/base-files.maintscript
@@ -0,0 +1 @@
+rm_conffile /etc/default/motd-news 11ubuntu11~ base-files
diff --git a/debian/changelog b/debian/changelog
index c72b59f..3fc05c8 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,19 @@
+base-files (11ubuntu11) groovy; urgency=medium
+
+  * Move the /etc/default/motd-news conffile to the motd-news-config
+    package (LP: #1888575):
+    - d/base-files.maintscript: remove /etc/default/motd-news config file
+      on upgrade
+    - d/control: break on ubuntu-server << 1.453 to force an upgrade if
+      it is installed, which will pull motd-news-config and the conffile
+      back in
+    - d/motd-news-config.postinst: handle the upgrade case where the
+      motd-news config file was changed while it belonged to base-files
+    - d/control: new motd-news-config package, carrying the
+      configuration file for the /etc/update-motd.d/50-motd-news script.
+
+ -- Andreas Hasenack <[e-mail]>  Thu, 06 Aug 2020 09:28:55 -0300
+
 base-files (11ubuntu10) groovy; urgency=medium
 
   * motd/50-motd-news: use the actual wget version variable in the
diff --git a/debian/control b/debian/control
index c90938a..d6b9138 100644
--- a/debian/control
+++ b/debian/control
@@ -14,7 +14,8 @@ Depends: ${misc:Depends}, ${shlibs:Depends}, libcrypt1 (>= 1:4.4.10-10ubuntu3)
 Essential: yes
 Priority: required
 Replaces: base, miscutils, dpkg (<= 1.15.0)
-Breaks: debian-security-support (<< 2019.04.25), initscripts (<< 2.88dsf-13.3), sendfile (<< 2.1b.20080616-5.2~)
+Breaks: debian-security-support (<< 2019.04.25), initscripts (<< 2.88dsf-13.3), sendfile (<< 2.1b.20080616-5.2~),
+ ubuntu-server (<< 1.453)
 Multi-Arch: foreign
 Description: Debian base system miscellaneous files
  This package contains the basic filesystem hierarchy of a Debian system, and
@@ -28,3 +29,14 @@ Section: debian-installer
 Architecture: all
 Priority: extra
 Description: LSB release information
+
+Package: motd-news-config
+Architecture: all
+Breaks: base-files (<< 11ubuntu11)
+Replaces: base-files (<< 11ubuntu11)
+Depends: ${shlibs:Depends}, ${misc:Depends}
+Description: Configuration for motd-news shipped in base-files
+ This package contains the configuration read by the motd-news script
+ shipped in the base-files package.
+ .
+ Install this package if you want motd-news to be enabled.
diff --git a/debian/motd-news-config.postinst b/debian/motd-news-config.postinst
 new file mode 100644
index 0000000..a5360c5
--- /dev/null
+++ b/debian/motd-news-config.postinst
@@ -0,0 +1,51 @@
+#!/bin/sh
+# postinst script for motd-news-config
+#
+# see: dh_installdeb(1)
+
+set -e
+
+# summary of how this script can be called:
+#        * <postinst> `configure' <most-recently-configured-version>
+#        * <old-postinst> `abort-upgrade' <new version>
+#        * <conflictor's-postinst> `abort-remove' `in-favour' <package>
+#          <new-version>
+#        * <postinst> `abort-remove'
+#        * <deconfigured's-postinst> `abort-deconfigure' `in-favour'
+#          <failed-install-package> <version> `removing'
+#          <conflicting-package> <version>
+# for details, see https://www.debian.org/doc/debian-policy/ or
+# the debian-policy package
+
+
+case "$1" in
+    configure)
+      # only run on new installs, $2 will be empty then
+      if [ -z "$2" ]; then
+          # /e/d/motd-news was moved from pkg:base-files to this package.
+          # base-files runs rm_conffile which, if it was modified, leaves
+          # /e/d/motd-news.dpkg-bak around. We want to preserve that *changed*
+          # config file in this migration, and this is something that
+          # rm_conffile is not handling.  In that case, let's put the backup
+          # file back in place
+          if [ -e /etc/default/motd-news.dpkg-bak ]; then
+              mv /etc/default/motd-news.dpkg-bak /etc/default/motd-news
+          fi
+      fi
+    ;;
+
+    abort-upgrade|abort-remove|abort-deconfigure)
+    ;;
+
+    *)
+        echo "postinst called with unknown argument \`$1'" >&2
+        exit 1
+    ;;
+esac
+
+# dh_installdeb will replace this with shell code automatically
+# generated by other debhelper scripts.
+
+#DEBHELPER#
+
+exit 0