From c7b0cc82fac79b47c3dd9a625cbd5a1eb192ed00 Mon Sep 17 00:00:00 2001
From: Michele Baldessari <michele@acksyn.org>
Date: Thu, 7 Nov 2019 12:01:41 +0100
Subject: [PATCH] Messaging notifications should be set as a list

We currently join the list into a comma-separated string.
This does not make a lot of sense because oslo messaging
notification drivers must not be passed in the driver=foo,bar form
but in the driver=foo\ndriver=bar form. See:

- https://docs.openstack.org/oslo.config/queens/configuration/format.html

multi valued (MultiStrOpt)

    A multi-valued option is a string value and can be given more than once, all values will be used.

    # Driver or drivers to handle sending notifications. (multi valued)
    notification_driver = nova.openstack.common.notifier.rpc_notifier
    notification_driver = ceilometer.compute.nova_notifier

By simply passing down the list we let all the puppet backends
that use the openstack_config:ruby backend to work correctly
(https://github.com/openstack/puppet-openstacklib#defined-provider-for-openstack_config-ruby)

This will fix the scenario where multiple notification drivers are
specified which is currently broken and will let the single driver
scenario to keep working as is.

Change-Id: If65946412b42e0919456ed92fdd8e3788ad67872
Related-Bug: #1851629
---
 manifests/messaging/notifications.pp          | 10 ++++++----
 .../oslo_messaging_notifications_spec.rb      | 19 ++++++++++++++++---
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/manifests/messaging/notifications.pp b/manifests/messaging/notifications.pp
index 45d0d34..9be67ae 100644
--- a/manifests/messaging/notifications.pp
+++ b/manifests/messaging/notifications.pp
@@ -10,7 +10,7 @@
 # [*driver*]
 #   (Optional) The Driver(s) to handle sending notifications.
 #   Possible values are messaging, messagingv2, routing, log, test, noop.
-#   (list value)
+#   (list value or string value)
 #   Defaults to $::os_service_default.
 #
 # [*transport_url*]
@@ -32,10 +32,12 @@ define oslo::messaging::notifications(
   $transport_url = $::os_service_default,
   $topics        = $::os_service_default,
 ) {
-  if !is_service_default($driver) {
-    $driver_orig = join(any2array($driver), ',')
-  } else {
+  if is_service_default($driver) or is_string($driver) {
+    # When we have a string value for driver,  we keep passing it as string
+    # to reduce any chance of breaking things in a backwards incompatible way
     $driver_orig = $driver
+  } else {
+    $driver_orig = any2array($driver)
   }
 
   if !is_service_default($topics) {
diff --git a/spec/defines/oslo_messaging_notifications_spec.rb b/spec/defines/oslo_messaging_notifications_spec.rb
index 6e87a81..8728bc3 100644
--- a/spec/defines/oslo_messaging_notifications_spec.rb
+++ b/spec/defines/oslo_messaging_notifications_spec.rb
@@ -17,15 +17,28 @@ describe 'oslo::messaging::notifications' do
 
     context 'with overridden parameters' do
       let :params do
-          { :driver        => ['messaging'],
+          { :driver        => ['messaging', 'messagingv2'],
             :transport_url => 'some_protocol://some_url',
+            :topics        => ['foo', 'baa'],
+          }
+      end
+
+      it 'configure oslo_messaging_notifications with overridden values' do
+        is_expected.to contain_keystone_config('oslo_messaging_notifications/driver').with_value(['messaging', 'messagingv2'])
+        is_expected.to contain_keystone_config('oslo_messaging_notifications/transport_url').with_value('some_protocol://some_url').with_secret(true)
+        is_expected.to contain_keystone_config('oslo_messaging_notifications/topics').with_value('foo,baa')
+      end
+    end
+
+    context 'with a single item in lists' do
+      let :params do
+          { :driver        => ['messaging'],
             :topics        => ['notifications'],
           }
       end
 
       it 'configure oslo_messaging_notifications with overridden values' do
-        is_expected.to contain_keystone_config('oslo_messaging_notifications/driver').with_value('messaging')
-        is_expected.to contain_keystone_config('oslo_messaging_notifications/transport_url').with_value('some_protocol://some_url').with_secret(true)
+        is_expected.to contain_keystone_config('oslo_messaging_notifications/driver').with_value(['messaging'])
         is_expected.to contain_keystone_config('oslo_messaging_notifications/topics').with_value('notifications')
       end
     end