fix parsing of primitive op parameters

libraries/pacemaker/resource/primitive.rb

@@ -32,7 +32,10 @@ class Pacemaker::Resource::Primitive < Pacemaker::Resource
       send(writer, hash)
     end
 
-    # FIXME: deal with op
+    self.op = {}
+    %w(start stop monitor).each do |op|
+      self.op[op] = self.class.extract_hash(definition, "op #{op}")
+    end
   end
 
   def params_string
@@ -50,9 +53,9 @@ class Pacemaker::Resource::Primitive < Pacemaker::Resource
   def definition_string
     return <<EOF
 primitive #{name} #{agent} \\
-        #{params_string} \\
+         #{params_string} \\
-        #{meta_string} \\
+         #{meta_string} \\
-        #{op_string}
+         #{op_string}
 EOF
   end
 
@@ -64,32 +67,28 @@ EOF
 
   def self.params_string(params)
     return "" if ! params or params.empty?
-    s = " params"
+    "params " +
-    params.sort.each do |key, value|
+    params.sort.map do |key, value|
-      s << %' #{key}="#{value}"'
+      %'#{key}="#{value}"'
-    end
+    end.join(' ')
-    s
   end
 
   def self.meta_string(meta)
     return "" if ! meta or meta.empty?
-    s = " meta"
+    "meta " +
-    meta.sort.each do |key, value|
+    meta.sort.map do |key, value|
-      s << %' #{key}="#{value}"'
+      %'#{key}="#{value}"'
-    end
+    end.join(' ')
-    s
   end
 
   def self.op_string(ops)
     return "" if ! ops or ops.empty?
-    s = " op"
+    ops.sort.map do |op, attrs|
-    ops.sort.each do |op, attrs|
+      attrs.empty? ? nil : "op #{op} " + \
-      s << " #{op}"
+      attrs.sort.map do |key, value|
-      attrs.sort.each do |key, value|
+        %'#{key}="#{value}"'
-        s << %' #{key}="#{value}"'
+      end.join(' ')
-      end
+    end.compact.join(' ')
-    end
-    s
   end
 
   # CIB object definitions look something like:
@@ -102,14 +101,16 @@ EOF
   #
   # This method extracts a Hash from one of the params / meta / op lines.
   def self.extract_hash(obj_definition, data_type)
-    unless obj_definition =~ /^\s+#{data_type} (.+?)(\s*\\)?$/
+    unless obj_definition =~ /\s+#{data_type} (.+?)\s*\\?$/
-      raise "Couldn't retrieve #{data_type} for '#{name}' CIB object from [#{obj_definition}]"
+      return {}
     end
 
     h = {}
     Shellwords.split($1).each do |kvpair|
+      break if kvpair == 'op'
       unless kvpair =~ /^(.+?)=(.+)$/
-        raise "Couldn't understand '#{kvpair}' for #{data_type} section of '#{name}' primitive"
+        raise "Couldn't understand '#{kvpair}' for '#{data_type}' section "\
+          "of #{name} primitive (definition was [#{obj_definition}])"
       end
       h[$1] = $2.sub(/^"(.*)"$/, "\1")
     end

spec/fixtures/keystone_primitive.rb

@@ -25,7 +25,7 @@ module Chef::RSpec
 primitive keystone ocf:openstack:keystone \
          params os_auth_url="http://node1:5000/v2.0" os_password="adminpw" os_tenant_name="openstack" os_username="admin" user="openstack-keystone" \
          meta is-managed="true" target-role="Started" \
-         op monitor interval="10s" timeout="60" start interval="10s" timeout="240"
+         op monitor interval="10s" timeout="60" op start interval="10s" timeout="240"
 EOF
     end
   end

spec/libraries/pacemaker/resource/primitive_spec.rb

@@ -8,6 +8,14 @@ describe Pacemaker::Resource::Primitive do
     Mixlib::ShellOut.any_instance.stub(:run_command)
   end
 
+  def expect_to_match_fixture(obj)
+    expect(obj.is_a? Pacemaker::Resource::Primitive).to be_true
+    %w(name agent params_string meta_string op_string).each do |field|
+      method = field.to_sym
+      expect(obj.send(method)).to eq(@primitive.send(method))
+    end
+  end
+
   it "should be instantiated via Pacemaker::CIBObject.from_name" do
     Mixlib::ShellOut.any_instance.stub(:error!)
     expect_any_instance_of(Mixlib::ShellOut) \
@@ -15,12 +23,12 @@ describe Pacemaker::Resource::Primitive do
       .and_return(@primitive.definition_string)
 
     obj = Pacemaker::CIBObject.from_name(@primitive.name)
-    expect(obj.is_a? Pacemaker::Resource::Primitive).to be_true
+    expect_to_match_fixture(obj)
   end
 
   it "should be instantiated via Pacemaker::CIBObject.from_definition" do
     obj = Pacemaker::CIBObject.from_definition(@primitive.definition_string)
-    expect(obj.is_a? Pacemaker::Resource::Primitive).to be_true
+    expect_to_match_fixture(obj)
   end
 
   it "should barf if the loaded definition's type is not primitive" do
@@ -49,7 +57,7 @@ describe Pacemaker::Resource::Primitive do
         "foo" => "bar",
         "baz" => "qux",
       }
-      expect(@primitive.params_string).to eq(%' params baz="qux" foo="bar"')
+      expect(@primitive.params_string).to eq(%'params baz="qux" foo="bar"')
     end
   end
 
@@ -69,7 +77,7 @@ describe Pacemaker::Resource::Primitive do
         "foo" => "bar",
         "baz" => "qux",
       }
-      expect(@primitive.meta_string).to eq(%' meta baz="qux" foo="bar"')
+      expect(@primitive.meta_string).to eq(%'meta baz="qux" foo="bar"')
     end
   end
 
@@ -91,15 +99,25 @@ describe Pacemaker::Resource::Primitive do
           "baz" => "qux",
         }
       }
-      expect(@primitive.op_string).to eq(%' op monitor baz="qux" foo="bar"')
+      expect(@primitive.op_string).to eq(%'op monitor baz="qux" foo="bar"')
     end
   end
 
   describe "::extract_hash" do
-    it "should extract a hash from config" do
+    it "should extract a params hash from config" do
       expect(@primitive.class.extract_hash(@primitive.definition_string, "params")).to \
         eq(Hash[@primitive.params])
     end
+
+    it "should extract an op start hash from config" do
+      expect(@primitive.class.extract_hash(@primitive.definition_string, 'op start')).to \
+        eq(Hash[@primitive.op]['start'])
+    end
+
+    it "should extract an op monitor hash from config" do
+      expect(@primitive.class.extract_hash(@primitive.definition_string, 'op monitor')).to \
+        eq(Hash[@primitive.op]['monitor'])
+    end
   end
 
   describe "#definition_string" do