vs_bridge: Use hash for external_ids

... so that data type is consistent with actual data type in ovs.

Closes-Bug: #2044136
Change-Id: I7b861d21d35d1886d64e47ea3c1f0fb15fa1c3af

lib/puppet/provider/vs_bridge/ovs.rb

@@ -46,13 +46,14 @@ Puppet::Type.type(:vs_bridge).provide(:ovs) do
   private
 
   def self.get_external_ids(br)
-    result = vsctl('br-get-external-id', br)
-    return result.split("\n").join(',')
+    value = vsctl('br-get-external-id', br)
+    value = value.split("\n").map{|i| i.strip}
+    return Hash[value.map{|i| i.split('=')}]
   end
 
   def self.set_external_ids(br, value)
-    old_ids = _split(get_external_ids(br))
-    new_ids = _split(value)
+    old_ids = get_external_ids(br)
+    new_ids = value
 
     new_ids.each do |k,v|
       if !old_ids.has_key?(k) or old_ids[k] != v
@@ -80,10 +81,6 @@ Puppet::Type.type(:vs_bridge).provide(:ovs) do
     vsctl('set', 'Bridge', br, "other-config:mac-table-size=#{value}")
   end
 
-  def self._split(string, splitter=',')
-    return Hash[string.split(splitter).map{|i| i.split('=')}]
-  end
-
   def self.get_bridge_other_config(br)
     value = vsctl('get', 'Bridge', br, 'other-config').strip
     value = value.gsub(/^{|}$/, '').split(',').map{|i| i.strip}

lib/puppet/type/vs_bridge.rb

@@ -18,12 +18,26 @@ Puppet::Type.newtype(:vs_bridge) do
   newproperty(:external_ids) do
     desc 'External IDs for the bridge: "key1=value2,key2=value2"'
 
-    validate do |value|
-      if !value.is_a?(String)
-        raise ArgumentError, "Invalid external_ids #{value}. Requires a String, not a #{value.class}"
+    munge do |value|
+      return value if value.is_a? Hash
+
+      internal = Hash.new
+      value.split(",").map{|el| el.strip()}.each do |pair|
+        key, value = pair.split("=", 2)
+        internal[key.strip()] = value.strip()
       end
-      if value !~ /^(?>[a-zA-Z]\S*=\S*){1}(?>[,][a-zA-Z]\S*=\S*)*$/
-        raise ArgumentError, "Invalid external_ids #{value}. Must a list of key1=value2,key2=value2"
+      return internal
+    end
+
+    validate do |value|
+      if value.is_a?(Hash)
+        true
+      elsif value.is_a?(String)
+        if value !~ /^(?>[a-zA-Z]\S*=\S*){1}(?>[,][a-zA-Z]\S*=\S*)*$/
+          raise ArgumentError, "Invalid external_ids #{value}. Must a list of key1=value2,key2=value2"
+        end
+      else
+        raise ArgumentError, "Invalid external_ids #{value}. Requires a String or a Hash, not a #{value.class}"
       end
     end
   end

releasenotes/notes/external-id-hash-cc61f1a3ab7f93ce.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - |
+    The ``external_ids`` property of the ``vs_bridge`` resource now supports
+    a hash value.

spec/acceptance/basic_vswitch_spec.rb

@@ -27,7 +27,7 @@ describe 'basic vswitch' do
 
       vs_bridge { 'br-ci4':
         ensure         => present,
-        external_ids   => 'bridge-id=br-ci4',
+        external_ids   => {'bridge-id' => 'br-ci4'},
         mac_table_size => 50000,
       }
       ~> exec { 'create_loop1_port':
@@ -42,6 +42,12 @@ describe 'basic vswitch' do
         bridge => 'br-ci4',
       }
 
+      vs_bridge { 'br-ci5':
+        ensure         => present,
+        external_ids   => 'bridge-id=br-ci5',
+        mac_table_size => 50000,
+      }
+
       vs_config { 'external_ids:ovn-remote':
         ensure => present,
         value  => 'tcp:127.0.0.1:2300',
@@ -113,6 +119,12 @@ describe 'basic vswitch' do
       end
     end
 
+    it 'should have external_ids on br-ci5 bridge' do
+      command('ovs-vsctl br-get-external-id br-ci5') do |r|
+        expect(r.stdout).to match(/bridge-id=br-ci5/)
+      end
+    end
+
     it 'should have mac-table-size on br-ci4 bridge' do
       command('ovs-vsctl get Bridge br-ci4 other-config:mac-table-size') do |r|
         expect(r.stdout).to match(/\"50000\"/)

spec/unit/provider/vs_bridge/ovs_spec.rb

@@ -48,7 +48,7 @@ describe Puppet::Type.type(:vs_bridge).provider(:ovs) do
     context 'with parameters' do
       before :each do
         resource_attrs.merge!(
-          :external_ids   => 'k=v',
+          :external_ids   => {'k' => 'v'},
           :mac_table_size => 60000,
         )
       end
@@ -91,7 +91,7 @@ describe Puppet::Type.type(:vs_bridge).provider(:ovs) do
       ).and_return(
         'k=v'
       )
-      expect(provider.external_ids).to eq('k=v')
+      expect(provider.external_ids).to eq({'k' => 'v'})
     end
   end
 
@@ -103,7 +103,7 @@ describe Puppet::Type.type(:vs_bridge).provider(:ovs) do
       expect(described_class).to receive(:vsctl).with(
         'br-set-external-id', 'testbr', 'k', 'v'
       )
-      provider.external_ids = 'k=v'
+      provider.external_ids = {'k' => 'v'}
     end
 
     it 'configures external ids when ids already exist' do
@@ -118,7 +118,7 @@ k3=v3')
       expect(described_class).to receive(:vsctl).with(
         'br-set-external-id', 'testbr', 'k3'
       )
-      provider.external_ids = 'k1=v1,k2=v2new'
+      provider.external_ids = {'k1' => 'v1', 'k2' => 'v2new'}
     end
   end
 

spec/unit/type/vs_bridge_spec.rb

@@ -3,9 +3,20 @@ require 'spec_helper'
 describe Puppet::Type.type(:vs_bridge) do
 
   it "should support present as a value for ensure" do
+    expect do
+      described_class.new(:name => 'foo', :ensure => :present)
+    end.to_not raise_error
+  end
+
+  it "should support a string value for external_ids" do
     expect do
       described_class.new(:name => 'foo', :ensure => :present, :external_ids => 'foo=br-ex,blah-id=bar)')
     end.to_not raise_error
   end
 
+  it "should support a hash value for external_ids" do
+    expect do
+      described_class.new(:name => 'foo', :ensure => :present, :external_ids => {'foo' => 'br-ex'})
+    end.to_not raise_error
+  end
 end