ok to consolidate classes?

[ghoneycutt]

Hi @dhoppe

The module currently separates out functionality that should be in one class (fail2ban) into many subclasses (install, config, service). Each class should provide functionality on its own without having to chain them together with the anchor pattern, which has come to be an anti-pattern in puppet coding. I'm looking at adding support for EL6 and ensuring there is full spec coverage and wondering if it would be OK to refactor the module to get rid of the anchor pattern and subclasses.

Best regards,
-g

[dhoppe]

@ghoneycutt Sure, I would appreciate your help. Just send me a PR and I am glad to merge your changes.

[ghoneycutt]

Thank you!

CC: @Phil-Friderici

[ghoneycutt]

Hi @dhoppe

@Phil-Friderici was able to consolidate in PR #21. Could you please merge?

[dhoppe]

@ghoneycutt I need some more days to take look at this pull request. Right now I am at a tropical island (honeymoon).

[Phil-Friderici]

@dhoppe happy honeymoon !!!

[Phil-Friderici]

Comment from @dhoppe in PR #21 (comment) :

@Phil-Friderici I know that this module need to be refactored and that the best practices change over time, but could you explain to me why you made these changes? I really do not like to put all the code into one file.

If I take a look at the PuppetConf talks the params.pp might be obsolete, but install/config/service is still valid and the differences between operating systems might be put into single files which are included if necessary.

[ghoneycutt]

Hi @dhoppe

I thought we were all on the same page for consolidating classes and the benefits from my original question above. We put a lot of resources into this based on your response and would very much appreciate it if you could merge PR #21

[dhoppe]

Hi @ghoneycutt

I am sorry we misunderstood. I have not read the text thoroughly enough and thought it was about the changes regarding Puppet 4. For example osfammily based manifest instead of params.pp and contain instead of the anchor pattern. I see no reason to put everything into one manifest because install.pp, config.pp, service.pp have established over time and in my opinion belong to the best practices.

[ghoneycutt]

Hi @dhoppe

Each class should provide functionality on its own. Breaking a class up into subclasses only makes the code harder to read. Since the subclasses do not stand on their own and only make sense between other classes, you end up having to go through all of them to understand what is happening. Sadly this approach was printed in a Puppet book and has since taken off from there.

While we differ on methods, I'm hoping that the simplified approach in the PR is still of enough value for you to accept the work and merge it.