Refactor layout

[petems]

@ghoneycutt Awesome refactor btw, was just going to do something like this and you've done everything I was gonna do! :smile:

[petems]

@floSoX Mind reviewing this? Pretty sweet refactor :+1:

[ghoneycutt]

Thanks @petems !

@Phil-Friderici and I are working on the spec tests to ensure we have comprehensive coverage. You can see the progress at

https://github.com/echoes-tech/puppet-monit/compare/master...ghoneycutt:add_tests?expand=1

[ghoneycutt]

We're doing this while ensuring we don't break backward compatibility, so you're current configuration and hiera data and such will still work.

[FlorentPoinsaut]

Hello,

Thank you so much for your hard work. The only point that I don't understand is why you delete the module structure (install.pp, config.pp, service.pp and firewall.pp). It is a best practice recommanded by PuppetLabs (https://docs.puppetlabs.com/guides/module_guides/bgtm.html) and I expect to conserve it in the future. Can you give me an explanation? Another time, thank you!

Regards,

[petems]

@floSoX The module pattern is useful but not essential. You could always merge this PR then refactor it back to use the separate classes pattern?

I would recommend is to only inherit from the params class in the init class, inheritance outside of the params pattern is not recomended: https://docs.puppetlabs.com/guides/style_guide.html#class-inheritance

Remember:

Class inheritance should only be used for myclass::params parameter defaults. Other use cases can be accomplished through the addition of parameters or conditional logic.

[ghoneycutt]

Closing in favor of PR #20 which includes tests.

[ghoneycutt]

Hi @floSoX It is a shame that Puppet Labs encourages people to use the pattern with the document to which you linked. It goes against their own style guide in terms of inheritance and promotes code that is hard to read while furthering terminology that is not compatible with an idempotent system that ensures state.

By having ::config, ::install, ::service, etc. You have multiple subclasses that can not stand on their own in terms of functionality. What you end up with is just a bunch of subclasses that you have to look through that together create a meaningful base class (in the init.pp). Subclasses are useful if they stand alone and do a specific thing than can be included from wherever. When all they do is a component of something else and are only meant to be called from a specific class, they end up just making more work than necessary and obfuscating the code.

My changes are not a critique of your hard work on this module but are sincerely to improve it so that we can accept it on our environment. I hope that even if we may disagree on the style issues, that the addition of all tests make PR #20 worth merging for you.

Thank you, -g

[Phil-Friderici]

@floSoX when I first saw params.pp I thought: nice way to specifiy the values of the params like you want to use it in your environment. But in every day reality I never changed params.pp for obvious reasons. Instead it started to get on my nerves when I need to look at this additional file all the time while coding functionality or spec tests.

Just my (biased :smirk:) 2 cents here

[petems]

@floSoX True, The full story is that it allows quick defaults for each OS, but also allows you to customise parameters. So if you need to support multi-OS for your module (as PuppetLabs often does) it's really useful for that.

WRT the config.pp, service.pp patterns, the breaking of the manifests into invididual parts is so the relationships are easier: anything in the config class refreshes any thing in the service class, so you can DRY-up your manifests, and not have to put a bunch of requirements on the individual resources.

[ghoneycutt]

As I said, I hope the additional quality of this code is enough to get it merged, even if we differ on style.

To address @petems comments, the ::params class is not needed to support many OS/version combinations. If you look at my approved modules[1], they support many platforms without using ::params.

Where ::params can make sense, is if you have multiple subclasses that need to access the same information. Instead of one class including another, which might not make any sense due to their differing natures, they could each include a ::params class to get shared information. Somehow this got misinterpreted along the way as all modules should have a params class.

I'm still against inheriting the ::params class though, as the only reason to ever use inheritance is when you want to override a resource. This is to satisfy DRY, instead of repeating the resource in another class.

[1] - https://forge.puppetlabs.com/modules?utf-8=%E2%9C%93&sort=rank&endorsements=approved&q=ghoneycutt

[ghoneycutt]

@petems and anyone else following. I forked this repo and released v1.0.0 which is compatible with this repo's v1.0.0, so you can port modules without worry.

https://github.com/ghoneycutt/puppet-module-monit/tree/v1.0.0

[petems]

@ghoneycutt Great! 👍