Add Param, Add hiera

[Kotty666]

needed if we've to deploy the whole gluster server in one Run.

Pull Request (PR) description
We needed the posibility to deploy the glusterfs-server at the first run, this won't work actually. So I've added the variable to force the binary path and disables the getvar() for the first run. With this we are able to add the Peers and Volume at the first run.

Also i've removed the params.pp and moved all to hiera.

This Pull Request (PR) fixes the following issues
#182 (if param force_binary is set)

[smortex]

Hi, thank you for this PR. I am not a user of gluster so might not be aware of some good reasons for relying on lookup, but since I consider it to be a code smell, I was alerted by the changes 😄

I added in-line notes to explain the issues to be aware of with lookup and how to avoid them. While here, I also added a few notes. Feel free to ask for more details/discuss the suggested changes and amend/add commits.

[smortex]

Do not add an explicit lookup for parameters. Here you are looking-up parameters for another class. Example of unexpected behavior:

class { 'gluster':
  client_package => 'foo',
}

class { 'gluster::client':
  # This will _NOT_ use 'foo' as the client package but whatever is in hiera
}

You can make these parameters optional, and after including gluster test them and if they are undef, fallback on the variables from init.pp; e.g.:

class gluster::client (
  Optional[String] $client_package,
) {
  include gluster

  $real_client_package = $client_package.lest { $gluster::client_package }
  # Use $real_client_package
}

But it might make more sense to skip completely the parameters in the class, and always use the variables from init.pp:

class gluster::client {
  include gluster

  # Use $gluster::client_package
}

[smortex]

Another suspect usage of lookup: please add a $service_enable parameter and directly reference this parameter.

Suggested change

	ensure => lookup('gluster::service_enable', Boolean, deep),
	ensure => $service_enable,

[smortex]

Optional with a default value will never be undef

Suggested change

	Optional[Boolean] $force_binary = false,
	Boolean $force_binary = false,

[smortex]

Huh… Is this basically checking if a fact is set or not a legacy way as suggested by the comment above?

If so, we should replace getvar() with fetching the fact from $facts and one more time, avoid the usage of lookup()

Suggested change

	if($force_binary) {
	$real_binary = getvar('::gluster_binary') ? {
	String => getvar('::gluster_binary'),
	default => lookup('gluster::gluster_binary',String,deep)
	}
	} else {
	$real_binary = getvar('::gluster_binary')
	}
	if ($force_binary) {
	$real_binary = $facts['gluster_binary'].lest || { $gluster::gluster_binary }
	} else {
	$real_binary = getvar('::gluster_binary')
	}

This look a bit odd, if what we really mean is "use the binary found by facter, and default to the binary supplied by the user, we can even remove the force_binary parameter completely and:

Suggested change

	if($force_binary) {
	$real_binary = getvar('::gluster_binary') ? {
	String => getvar('::gluster_binary'),
	default => lookup('gluster::gluster_binary',String,deep)
	}
	} else {
	$real_binary = getvar('::gluster_binary')
	}
	$real_binary = $facts['gluster_binary'].lest || { $gluster::gluster_binary }

[smortex]

Same code as in gluster::peer… It's probably simpler to define this variable once in init.pp and use $gluster::real_binary in both gluster::peer, gluster::volume and gluster::volume::option.

[smortex]

No need to add explicit lookup(): puppet does an implicit lookup for each undef parameter. Note however that the parameter client fetch the install_client value form hiera which does not match it's name.

Suggested change

	Boolean $client = lookup('gluster::install_client', Boolean, deep),
	$client_package = lookup('gluster::client_package', String, deep),
	$pool = lookup('gluster::pool', String, deep),
	$repo = lookup('gluster::repo', String, deep),
	$release = lookup('gluster::release', String, deep),
	$server = lookup('gluster::install_server', String, deep),
	$server_package = lookup('gluster::server_package', String, deep),
	$use_exported_resources = lookup('gluster::export_resources', String, deep),
	$version = lookup('gluster::version', String, deep),
	Boolean $client,
	$client_package,
	$pool,
	$repo,
	$release,
	$server,
	$server_package,
	$use_exported_resources,
	$version,

But while here, you can add data types in front of each parameter 👍

[smortex]

@Kotty666 you seem to be actively working on this and your changes do not seem ready to merge ATM, so I will close the PR for now so that we are not notified each time you push changes.

When you are done with your change, feel free to squash your commits, rebase them on top of master and re-open this PR so that we can follow-up.

Thanks!