Fixed ovs provider

The vs_bridge type needed:

  - A regex value to validate external_ids

  - An extra test for autorequire

The vs_bridge provider needed:

  - Openvswitch corresponding link to go
    down before being remove: #destroy

  - parameter external_ids had a typo and
    couldn't be setup properly during create

Standardization

 - Replaced 'optional_commands' with 'commands'
   and used no path to be more generic as
   puppet will search for best path

 - Types using :namevar good practice

 - Inline documentation: for all parameters
   and properties. Replaced @doc with desc

 - Comestic: some doubled quotes => single

Change-Id: I522888b1d2d147e07667344f9dfc9bcbc0c96668

lib/puppet/provider/vs_bridge/ovs.rb

@@ -1,8 +1,8 @@
-require "puppet"
+require 'puppet'
 
 Puppet::Type.type(:vs_bridge).provide(:ovs) do
-  optional_commands :vsctl => "/usr/bin/ovs-vsctl",
-                    :ip    => "/sbin/ip"
+  commands :vsctl => 'ovs-vsctl'
+  commands :ip    => 'ip'
 
   def exists?
     vsctl("br-exists", @resource[:name])
@@ -11,22 +11,23 @@ def exists?
   end
 
   def create
-    vsctl("add-br", @resource[:name])
-    ip("link", "set", @resource[:name], "up")
-    external_ids = @resource[:external_ids] if@resource[:external_ids]
+    vsctl('add-br', @resource[:name])
+    ip('link', 'set', @resource[:name], 'up')
+    external_ids = @resource[:external_ids] if @resource[:external_ids]
   end
 
   def destroy
-    vsctl("del-br", @resource[:name])
+    ip('link', 'set', @resource[:name], 'down')
+    vsctl('del-br', @resource[:name])
   end
 
-  def _split(string, splitter=",")
-    return Hash[string.split(splitter).map{|i| i.split("=")}]
+  def _split(string, splitter=',')
+    return Hash[string.split(splitter).map{|i| i.split('=')}]
   end
 
   def external_ids
-    result = vsctl("br-get-external-id", @resource[:name])
-    return result.split("\n").join(",")
+    result = vsctl('br-get-external-id', @resource[:name])
+    return result.split("\n").join(',')
   end
 
   def external_ids=(value)
@@ -35,7 +36,7 @@ def external_ids=(value)
 
     new_ids.each_pair do |k,v|
       unless old_ids.has_key?(k)
-        vsctl("br-set-external-id", @resource[:name], k, v)
+        vsctl('br-set-external-id', @resource[:name], k, v)
       end
     end
   end

lib/puppet/provider/vs_port/ovs.rb

@@ -1,17 +1,21 @@
-require "puppet"
+require 'puppet'
 
 Puppet::Type.type(:vs_port).provide(:ovs) do
-  optional_commands :vsctl => "/usr/bin/ovs-vsctl"
+  desc 'Openvswitch port manipulation'
+
+  commands :vsctl => 'ovs-vsctl'
 
   def exists?
-    vsctl("list-ports", @resource[:bridge]).include? @resource[:interface]
+    vsctl('list-ports', @resource[:bridge]).include? @resource[:interface]
+  rescue Puppet::ExecutionFailure => e
+    return false
   end
 
   def create
-    vsctl("add-port", @resource[:bridge], @resource[:interface])
+    vsctl('add-port', @resource[:bridge], @resource[:interface])
   end
 
   def destroy
-    vsctl("del-port", @resource[:bridge], @resource[:interface])
+    vsctl('del-port', @resource[:bridge], @resource[:interface])
   end
 end

lib/puppet/type/vs_bridge.rb

@@ -1,19 +1,30 @@
-require "puppet"
+require 'puppet'
 
-module Puppet
-  Puppet::Type.newtype(:vs_bridge) do
-    @doc = "A Switch - For example 'br-int' in OpenStack"
+Puppet::Type.newtype(:vs_bridge) do
+  desc 'A Switch - For example "br-int" in OpenStack'
 
-    ensurable
+  ensurable
 
-    newparam(:name) do
-      isnamevar
-      desc "The bridge to configure"
-    end
+  newparam(:name, :namevar => true) do
+    desc 'The bridge to configure'
 
-    newproperty(:external_ids) do
-      desc "External IDs for the bridge"
+    validate do |value|
+      if !value.is_a?(String)
+        raise ArgumentError, "Invalid name #{value}. Requires a String, not a #{value.class}"
+      end
     end
+  end
 
+  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}"
+      end
+      if value !~ /^(?>[a-zA-Z]\w*=\w*){1}(?>[,][a-zA-Z]\w*=\w*)*$/
+        raise ArgumentError, "Invalid external_ids #{value}. Must a list of key1=value2,key2=value2"
+      end
+    end
   end
 end

lib/puppet/type/vs_port.rb

@@ -1,43 +1,49 @@
-require "puppet"
+require 'puppet'
 
-module Puppet
-  Puppet::Type.newtype(:vs_port) do
-    @doc = "A Virtual Switch Port"
+Puppet::Type.newtype(:vs_port) do
+  desc 'A Virtual Switch Port'
 
-    ensurable
+  ensurable
 
-    newparam(:interface) do
-      isnamevar
-      desc "The interface to attach to the bridge"
-    end
+  newparam(:interface, :namevar => true) do
+    desc 'The interface to attach to the bridge'
 
-    newparam(:bridge) do
-      desc "What bridge to use"
+    validate do |value|
+      if !value.is_a?(String)
+        raise ArgumentError, "Invalid interface #{value}. Requires a String, not a #{value.class}"
+      end
     end
+  end
 
-    # newparam(:keep_ip, :boolean => true, :parent => Puppet::Parameter::Boolean) do
-    newparam(:keep_ip) do
-      desc "True: keep physical interface's details and assign them to the bridge" 
+  newparam(:bridge) do
+    desc "What bridge to use"
 
-      defaultto false
+    validate do |value|
+      if !value.is_a?(String)
+        raise ArgumentError, "Invalid bridge #{value}. Requires a String, not a #{value.class}'"
+      end
     end
+  end
+
+  newparam(:keep_ip) do
+    desc "True: keep physical interface's details and assign them to the bridge"
 
-    newparam(:sleep) do
-      desc "Waiting time, in seconds (0 by default), for network to sync after activating port, used with keep_ip only"
+    defaultto false
+  end
+
+  newparam(:sleep) do
+    desc "Waiting time, in seconds (0 by default), for network to sync after activating port, used with keep_ip only"
+
+    defaultto '0'
 
-      defaultto '0'
-
-      validate do |value|
-        if value.to_i.class != Fixnum || value.to_i < 0
-          raise ArgumentError, "sleep requires a positive integer"
-        end
+    validate do |value|
+      if value.to_i.class != Fixnum || value.to_i < 0
+        raise ArgumentError, "sleep requires a positive integer"
       end
     end
-
-    autorequire(:vs_bridge) do
-      [self[:bridge]]
-    end
+  end
 
+  autorequire(:vs_bridge) do
+    self[:bridge] if self[:bridge]
   end
 end
-