Code review comment for ~ahasenack/ubuntu/+source/base-files:groovy-motd-news-config-split

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Great testing as always.
I was not looking at re-running your tests, instead I wondered "what could happen" and a less surgical blunt user came to mind that just deleted /etc/default/motd-news which at first achieves the same thing (motd news is off)

I tested that on ubuntu-server and without ubuntu-server.

Without Ubuntu-server it is ok, the file does not come back and stays disabled.

But with Ubuntu-server installed there is nothing rm_conffile of base-files does.
Therefore no artifact is left, and due to that the default file is placed (again) and thereby re-enabling the service :-/

# Working (default)

root@g2:~# bash -x /etc/update-motd.d/50-motd-news
+ '[' -r /etc/default/motd-news ']'
+ . /etc/default/motd-news
++ ENABLED=1
++ URLS=https://motd.ubuntu.com
++ WAIT=5
+ '[' 1 = 1 ']'
+ '[' -n https://motd.ubuntu.com ']'
+ '[' -n 5 ']'
+ '[' -n '' ']'
+ CACHE=/var/cache/motd-news
+ '[' '' = --force ']'
+ '[' '' '!=' 1 ']'
+ '[' -r /var/cache/motd-news ']'
++ id -u
+ '[' 0 -eq 0 ']'
+ :
+ exit 0

# Remove file (disabled)

root@g2:~# rm /etc/default/motd-news
root@g2:~# bash -x /etc/update-motd.d/50-motd-news
+ '[' -r /etc/default/motd-news ']'
+ '[' '' = 1 ']'
+ exit 0

# Upgrade re-enables

root@g2:~# add-apt-repository ppa:ahasenack/zomg-2

 More info: https://launchpad.net/~ahasenack/+archive/ubuntu/zomg-2
Press [ENTER] to continue or Ctrl-c to cancel adding it.

Hit:1 http://archive.ubuntu.com/ubuntu groovy InRelease
Hit:2 http://archive.ubuntu.com/ubuntu groovy-updates InRelease
Hit:3 http://archive.ubuntu.com/ubuntu groovy-backports InRelease
Get:4 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy InRelease [17.5 kB]
Hit:5 http://security.ubuntu.com/ubuntu groovy-security InRelease
Get:6 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy/main amd64 Packages [3772 B]
Get:7 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy/main Translation-en [1056 B]
Fetched 22.3 kB in 1s (27.7 kB/s)
Reading package lists... Done
root@g2:~#
root@g2:~#
root@g2:~# apt upgrade
Reading package lists... Done
Building dependency tree
Reading state information... Done
Calculating upgrade... Done
The following package was automatically installed and is no longer required:
  libfreetype6
Use 'apt autoremove' to remove it.
The following NEW packages will be installed:
  motd-news-config
The following packages will be upgraded:
  base-files ubuntu-minimal ubuntu-server ubuntu-standard
4 upgraded, 1 newly installed, 0 to remove and 0 not upgraded.
Need to get 262 kB of archives.
After this operation, 45.1 kB of additional disk space will be used.
Do you want to continue? [Y/n] Y
Get:1 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy/main amd64 ubuntu-server amd64 1.453 [46.8 kB]
Get:2 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy/main amd64 base-files amd64 11ubuntu11 [88.6 kB]
Get:3 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy/main amd64 motd-news-config all 11ubuntu11 [32.5 kB]
Get:4 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy/main amd64 ubuntu-minimal amd64 1.453 [46.9 kB]
Get:5 http://ppa.launchpad.net/ahasenack/zomg-2/ubuntu groovy/main amd64 ubuntu-standard amd64 1.453 [46.9 kB]
Fetched 262 kB in 1s (340 kB/s)
(Reading database ... 31477 files and directories currently installed.)
Preparing to unpack .../ubuntu-server_1.453_amd64.deb ...
Unpacking ubuntu-server (1.453) over (1.452) ...
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 (11ubuntu10) ...
Setting up base-files (11ubuntu11) ...
motd-news.service is a disabled or a static unit, not starting it.
Selecting previously unselected package motd-news-config.
(Reading database ... 31476 files and directories currently installed.)
Preparing to unpack .../motd-news-config_11ubuntu11_all.deb ...
Unpacking motd-news-config (11ubuntu11) ...
Preparing to unpack .../ubuntu-minimal_1.453_amd64.deb ...
Unpacking ubuntu-minimal (1.453) over (1.452) ...
Preparing to unpack .../ubuntu-standard_1.453_amd64.deb ...
Unpacking ubuntu-standard (1.453) over (1.452) ...
Setting up motd-news-config (11ubuntu11) ...
Setting up ubuntu-minimal (1.453) ...
Setting up ubuntu-standard (1.453) ...
Setting up ubuntu-server (1.453) ...
Processing triggers for man-db (2.9.3-2) ...
Processing triggers for plymouth-theme-ubuntu-text (0.9.4git20200323-0ubuntu6) ...
update-initramfs: deferring update (trigger activated)
Processing triggers for install-info (6.7.0.dfsg.2-5) ...
Processing triggers for initramfs-tools (0.137ubuntu10) ...
root@g2:~# bash -x /etc/update-motd.d/50-motd-news
+ '[' -r /etc/default/motd-news ']'
+ . /etc/default/motd-news
++ ENABLED=1
++ URLS=https://motd.ubuntu.com
++ WAIT=5
+ '[' 1 = 1 ']'
+ '[' -n https://motd.ubuntu.com ']'
+ '[' -n 5 ']'
+ '[' -n '' ']'
+ CACHE=/var/cache/motd-news
+ '[' '' = --force ']'
+ '[' '' '!=' 1 ']'
+ '[' -r /var/cache/motd-news ']'
+ echo

+ safe_print /var/cache/motd-news
+ cat /var/cache/motd-news
+ head -n 10
+ tr -d '\000-\011\013\014\016-\037'
+ cut -c -80
+ exit 0

What do you think, does this require a bit more maintscript magic to be covered as well?

other than that things look +1 to me.
Also I like Steves suggestion to stay in one src:package so we only have NEW for the binary.

review: Needs Information

« Back to merge proposal