fix parameter lookup

[saz]

Since the merge of #322 it wasn't possible to just manage server ssh::server or client ssh::client with this module.

This PR moves all parameters to the proper location (thanks to #325 this module uses data in modules), which should make things work as before again.

I've also added a few more tests in this PR

Fixes #324
Fixes #326
Fixes #329

[SimonHoenscheid]

Hi @saz, first of all, sorry if my PR has caused trouble. That was not my intention.

I had a look at this PR and I try to sum up what I understood:

The reason for this PR and my PR which declared the ssh::client and ssh::server class as private was to solve the duplicate declaration which started, when I moved the module to Data in Modules.

From my understanding of the Puppet language it is best practice to introduce all variables as class parameters in the main class and access this parameter from a subclass to load the value.

Before introducing Data in Modules the default parameters were stored in the params.pp and were loaded from there. This was best practice, when params.pp was the way to go and even back then a lot of modules loaded the defaults to the init.pp and accessed the variable from an other class.

When declaring the module with a simple include every seperate declaration of the client or server class will be a duplicate, because, init.pp is already loading it.

Loading the parameters from the subclass looks uncommon for me, even if it solves the problem and can be overwritten from the main class.

Just a few thoughts
Simon

[saz]

Hi @SimonHoenscheid

Your understanding is correct and I don't see any trouble here 😄 Test should have been improved a long time ago and would have make this issue visible before anything gets merged.

That's what I did now: tests should reflect the behavior of this module.

The main class of this module is - and always has been - some kind of convenience thing for the majority of use-cases: manage both client and server in a standard way by just include ssh.
It's discussable if this is good or bad, but at the same time, there are a couple of modules doing the same.

There are still a few things, I'm not sure, if it should stays like that or if it should be changed.
One example from manifests/server/config.pp:

class ssh::server::config {
  assert_private()

  $options = $ssh::server::merged_options

$options might also be a class parameter and instead of doing an include ssh::server::config, it's possible to just do a

class { 'ssh::server::config':
  options => $merged_options,
}

which will make things even more configurable.

I appreciate the time you're investing and if you still have any feedback on this PR, it's more than welcome.

I'm planning on merging this on the weekend.
@smoeding Already provided some feedback in #324 which helped in fixing an issue I would have introduced with this PR.

I'm (almost) happy with the way this PR looks (even if it's huge!) and how everything's working (hopefully: as before v9 😄 )

[SimonHoenscheid]

private API marking needs removal in L1, server.pp

[saz]

PR merged 🚀