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

RESOLVED WONTFIX

Status

Infrastructure & Operations
Infrastructure: Other
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: dustin, Assigned: dustin)

Tracking

Details

Attachments

(1 attachment)

Ported back from puppetagain.

It occurs to me way may also want to include motd::base in the base class?
Created attachment 719131 [details] [diff] [review]
bug845951.patch
Attachment #719131 - Flags: review?(rsoderberg)
I'm not very familiar with concat. Will this wipe out any existing motd?
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.
Richard, ping?
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.
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 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.
Attachment #719131 - Flags: review?(rsoderberg) → review-
OK :)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
Component: Server Operations: Infrastructure → Infrastructure: Other
Product: mozilla.org → Infrastructure & Operations
You need to log in before you can comment on or make changes to this bug.