Fix correcting option

functions/cmd_volume_get_option.pp

@@ -0,0 +1,34 @@
+# Create a command string to get option `$opt` from gluster volume `$vol`, and
+# optionally compare it against `$comparison`.
+#
+# @param vol [Gluster::VolumeName] Gluster volume name
+# @param opt [Gluster::OptionName] Gluster volume option name
+# @param comparison [Optional[String]] Optional string to compare the existing
+#   value against
+# @return [String]
+#
+# @example Usage
+#
+#   ```puppet
+#   gluster::cmd_volume_get_option('data', 'nfs.disable', String(true))
+#   ```
+#
+function gluster::cmd_volume_get_option(
+  Gluster::VolumeName   $vol,
+  Gluster::VolumeOption $opt,
+  Optional[Any]         $comparison = undef,
+) {
+  $_cmd = "${facts['gluster_binary']} volume get ${vol} ${opt}"
+
+  unless $comparison {
+    return $_cmd
+  }
+
+  $_comparison = $comparison ? {
+    Undef   => '\(null\)',
+    Boolean => gluster::onoff($comparison),
+    default => $comparison,
+  }
+
+  "${_cmd} | tail -n1 | grep -E '^${opt} +${_comparison} *\$'"
+}

functions/onoff.pp

@@ -0,0 +1,9 @@
+function gluster::onoff (
+  Boolean $value,
+) {
+  if $value {
+    'on'
+  } else {
+    'off'
+  }
+}

manifests/peer.pp

@@ -39,19 +39,20 @@
   $fqdn = $::fqdn,
 ) {
 
-  # we can't do much without the Gluster binary
-  # but we don't necessarily want the Puppet run to fail if the
-  # gluster_binary fact is absent!
-  if getvar('::gluster_binary') {
+  # we can't do much without the Gluster binary but we don't necessarily want
+  # the Puppet run to fail if the gluster_binary fact is absent!
+  $_gluster_binary = $facts.dig('gluster_binary')
+  if $_gluster_binary {
     # we can't join to ourselves, so it only makes sense to operate
     # on other gluster servers in the same pool
     if $fqdn != $::fqdn {
 
-      # and we don't want to attach a server that is already a member
-      # of the current pool
-      if getvar('::gluster_peer_list') {
-        $peers = split($::gluster_peer_list, ',' )
-        if ! member($peers, $title) {
+      # and we don't want to attach a server that is already a member of the
+      # current pool
+      $_gluster_peer_list = $facts.dig('gluster_peer_list')
+      if $_gluster_peer_list {
+        $peers = split($_gluster_peer_list, ',' )
+        if $title in $peers {
           $already_in_pool = false
         } else {
           $already_in_pool = true
@@ -61,7 +62,7 @@
       }
       if !$already_in_pool {
         exec { "gluster peer probe ${title}":
-            command => "${::gluster_binary} peer probe ${title}",
+            command => "${_gluster_binary} peer probe ${title}",
           }
       }
     }

manifests/volume.pp

@@ -99,20 +99,18 @@
 
   $args = join(delete($cmd_args, ''), ' ')
 
-  if getvar('::gluster_binary'){
+  $_gluster_binary = $facts.dig('gluster_binary')
+  if $_gluster_binary {
     # we need the Gluster binary to do anything!
 
-    if getvar('::gluster_volume_list') and member( split( $::gluster_volume_list, ',' ), $title ) {
-      $already_exists = true
-    } else {
-      $already_exists = false
-    }
+    $_gluster_volume_list = $facts.dig('gluster_volume_list')
+    $already_exists = $_gluster_volume_list and $title in $_gluster_volume_list.split(',')
 
-    if $already_exists == false {
+    unless $already_exists {
       # this volume has not yet been created
 
       exec { "gluster create volume ${title}":
-        command => "${::gluster_binary} volume create ${title} ${args}",
+        command => "${_gluster_binary} volume create ${title} ${args}",
       }
 
       # if we have volume options, activate them now
@@ -144,20 +142,22 @@
           before  => Exec["gluster start volume ${title}"],
         }
 
-        create_resources(::gluster::volume::option, $hoh, $new_volume_defaults)
+        create_resources('::gluster::volume::option', $hoh, $new_volume_defaults)
       }
 
       # don't forget to start the new volume!
       exec { "gluster start volume ${title}":
-        command => "${::gluster_binary} volume start ${title}",
+        command => "${_gluster_binary} volume start ${title}",
         require => Exec["gluster create volume ${title}"],
       }
 
-    } elsif $already_exists {
+    } else {
       # this volume exists
 
       # our fact lists bricks comma-separated, but we need an array
-      $vol_bricks = split( getvar( "::gluster_volume_${title}_bricks" ), ',')
+      $vol_bricks = $facts.dig("gluster_volume_${title}_bricks").then |$bs| {
+        $bs.split(',')
+      }
       if $bricks != $vol_bricks {
         # this resource's list of bricks does not match the existing
         # volume's list of bricks
@@ -193,12 +193,12 @@
 
           $new_bricks_list = join($new_bricks, ' ')
           exec { "gluster add bricks to ${title}":
-            command => "${::gluster_binary} volume add-brick ${title} ${s} ${r} ${new_bricks_list} ${_force}",
+            command => "${_gluster_binary} volume add-brick ${title} ${s} ${r} ${new_bricks_list} ${_force}",
           }
 
           if $rebalance {
             exec { "gluster rebalance ${title}":
-              command => "${::gluster_binary} volume rebalance ${title} start",
+              command => "${_gluster_binary} volume rebalance ${title} start",
               require => Exec["gluster add bricks to ${title}"],
             }
           }
@@ -208,7 +208,7 @@
           # the self heal daemon comes back to life.
           # as such, we sleep 5 here before starting the heal
             exec { "gluster heal ${title}":
-              command => "/bin/sleep 5; ${::gluster_binary} volume heal ${title} full",
+              command => "/bin/sleep 5; ${_gluster_binary} volume heal ${title} full",
               require => Exec["gluster add bricks to ${title}"],
             }
           }
@@ -222,15 +222,14 @@
       }
 
       # did the options change?
-      $current_options_hash = pick(fact("gluster_volumes.${title}.options"), {})
-      $_current = sort(join_keys_to_values($current_options_hash, ': '))
-
+      $current_options = $facts.dig("gluster_volume_${title}.options").pick({})
+      $_current = sort(join_keys_to_values($current_options, ': '))
       if $_current != $_options {
         #
         # either of $current_options or $_options may be empty.
         # we need to account for this situation
         #
-        if is_array($_current) and is_array($_options) {
+        if $_current =~ Array and $_options =~ Array {
           $to_remove = difference($_current, $_options)
           $to_add = difference($_options, $_current)
         } else {
@@ -242,7 +241,7 @@
             $to_add = $_options
           }
         }
-        if ! empty($to_remove) {
+        unless $to_remove.empty {
           # we have some options active that are not defined here. Remove them
           #
           # the syntax to remove ::gluster::volume::options is a little different
@@ -252,18 +251,18 @@
           $remove_yaml = join( regsubst( $remove_opts, ': .+$', ":\n  ensure: absent", 'G' ), "\n" )
           $remove = parseyaml($remove_yaml)
           if $remove_options {
-            create_resources( ::gluster::volume::option, $remove )
+            create_resources('::gluster::volume::option', $remove )
           } else {
             $remove_str = join( keys($remove), ', ' )
             notice("NOT REMOVING the following options for volume ${title}: ${remove_str}.")
           }
         }
-        if ! empty($to_add) {
+        unless $to_add.empty {
           # we have some options defined that are not active. Add them
           $add_opts = prefix( $to_add, "${title}:" )
           $add_yaml = join( regsubst( $add_opts, ': ', ":\n  value: ", 'G' ), "\n" )
           $add = parseyaml($add_yaml)
-          create_resources( ::gluster::volume::option, $add )
+          create_resources('::gluster::volume::option', $add )
         }
       }
     }

manifests/volume/option.pp

@@ -3,7 +3,8 @@
 # @param title
 #    the name of the volume, a colon, and the name of the option
 # @param value
-#    the value to set for this option
+#    the value to set for this option. Boolean values will be coerced to
+#    'on'/'off'.
 # @param ensure
 #    whether to set or remove an option
 #
@@ -30,31 +31,29 @@
   Enum['present', 'absent']                   $ensure = 'present',
 ) {
 
-  $arr = split( $title, ':' )
-  $count = count($arr)
+  $arr = $title.split(':')
   # do we have more than one array element?
-  if $count != 2 {
+  if count($arr) != 2 {
     fail("${title} does not parse as volume:option")
   }
-  $vol = $arr[0]
-  $opt = $arr[1]
+  [$vol, $opt] = $arr
+
+  $_value = $value ? {
+    Boolean => gluster::onoff($value),
+    default => String($value),
+  }
 
   $cmd = if $ensure == 'absent' {
     "reset ${vol} ${opt}"
   } else {
-    "set ${vol} ${opt} ${value}"
-  }
-
-  $_value = $value ? {
-    Boolean => if $value {
-      'on'
-    } else {
-      'off'
-    },
-    default => $value,
+    "set ${vol} ${opt} ${_value}"
   }
 
   exec { "gluster option ${vol} ${opt} ${_value}":
+    path    => '/usr/bin:/usr/sbin:/bin',
     command => "${facts['gluster_binary']} volume ${cmd}",
+    unless  => unless $ensure == 'absent' {
+      gluster::cmd_volume_get_option($vol, $opt, $_value)
+    },
   }
 }

types/volumename.pp

@@ -0,0 +1 @@
+type Gluster::VolumeName = Pattern[/^[a-zA-Z0-9_-]+$/]

types/volumeoption.pp

@@ -0,0 +1 @@
+type Gluster::VolumeOption = Pattern[/^[a-z0-9]+\.[a-z0-9-]+$/]