845951 - Don't fail when including motd::base but no motd entries

Status: RESOLVED WONTFIX

(Infrastructure & Operations :: Infrastructure: Other, task)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

Ported back from puppetagain.

It occurs to me way may also want to include motd::base in the base class?

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug845951.patch

Comment 2

[:Atoll]

I'm not very familiar with concat. Will this wipe out any existing motd?

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

If motd::base is included, yes, it will set an empty motd.  So, yes, adding this to the base class will wipe out any non-puppet-controlled motd's.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Richard, ping?

Comment 5

[:Atoll]

We can't include this by default, since it would interfere with motd changes everywhere.

To import this change, we'd need to verify that it would have no harmful effects on the MOTDs of the following systems:

modules/gunicorn/manifests/init.pp:    motd {
modules/gunicorn/manifests/instance.pp:    motd {
modules/mysql2/manifests/server.pp:    motd {
modules/releng/manifests/puppetmaster/data.pp:        motd {
modules/releng/manifests/puppetmaster/data.pp:        motd {
modules/webapp/manifests/admin.pp:    motd {
modules/webapp/manifests/admin/releng.pp:    motd {
modules/webapp/manifests/releng/prod.pp:    motd {

Does this change remove any *existing* motd and replace it only with the ::fragment provided? Or, without it, is the motd {} class completely broken entirely?

I'm still unclear how things are working today for all of the above-listed consumers of it when it's apparently completely broken without.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Those motd's will stay in place, since they're in puppet

Concat has two parts: you define a target file, and then you define fragments for that file.  It fails if you define 0 fragments for a target.

The use-case in puppetagain is this:

puppet::motd {
+    include motd::base
+
+    if ($pin_puppet_env != '') {
+        motd {
+            pinned-puppet-env:
+                content => "NOTE: puppet environment pinned to '$pin_puppet_env'\n";
+        }
+    }
+    if ($pin_puppet_server != '') {
+        motd {
+            pinned-puppet-server:
+                content => "NOTE: puppet server pinned to '$pin_puppet_server'\n";
+        }
+    }
+}

if both conditionals are false, then I want to enforce an empty /etc/motd.  Without this patch, the puppet run fails instead.

My response in comment 3 was unclear, because nobody has suggested adding this to the base class.  This patch should have no effect on managed or unmanaged /etc/motd files.

Comment 7

[:Atoll]

Comment on attachment 719131 [details] [diff] [review]
bug845951.patch

(In reply to Dustin J. Mitchell [:dustin] from comment #6)
> if both conditionals are false, then I want to enforce an empty /etc/motd. 
> Without this patch, the puppet run fails instead.

I don't like the idea of motd::base automatically wiping out /etc/motd when no fragments are defined. In fact, I strongly prefer having it fail, which alerts the user that their code is incomplete.

In your example, adding the empty fragment to puppet::motd *is* a valid and correct solution -- and it clearly states in the class that /etc/motd will be empty by default, unless those things are set.

So if this was off-by-default, with a class parameter to enable it, that would be fine. However in every other case, failure to provide a motd {} fragment means that a coding error has occurred.

(In reply to Dustin J. Mitchell [:dustin] from comment #6)
> My response in comment 3 was unclear, because nobody has suggested adding
> this to the base class.  This patch should have no effect on managed or
> unmanaged /etc/motd files.

It's in reply to this comment:

(In reply to Dustin J. Mitchell [:dustin] from comment #0)
> It occurs to me way may also want to include motd::base in the base class?

We can't include motd::base by default, since most servers have no motd {} fragments, and *all* infra servers have local changes to /etc/motd due to puppetctl (among others) that must not be wiped out by this.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

OK :)