Refactor to simplify code #53
Conversation
manifests/init.pp
Outdated
| owner => $caddy_user, | ||
| group => $caddy_group, | ||
| mode => '0444', | ||
| source => 'puppet:///modules/caddy/etc/caddy/Caddyfile', |
There was a problem hiding this comment.
I have become a fan of
| source => 'puppet:///modules/caddy/etc/caddy/Caddyfile', | |
| content => file('caddy/etc/caddy/Caddyfile'), |
There was a problem hiding this comment.
me too, just trying to focus on changing the design pattern with the sub classes. I'll go ahead and change this.
There was a problem hiding this comment.
Realized that the file system layout was used under files/ and templates/.. Going to clean that up, too. Different platforms place things in different places, so this pattern should not be used.
There was a problem hiding this comment.
Agreed, but it does blow up the size of the PR. Perhaps that should be its own so it's easier to review?
There was a problem hiding this comment.
It's pretty simple and straight forward since there are only two files in each of files/ and templates/ that were moved.
|
Dear @ghoneycutt, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
| caddy::vhost {'example1': | ||
| source => 'puppet:///modules/caddy/etc/caddy/config/example1.conf', | ||
| caddy::vhost { 'example1': | ||
| content => file('caddy/examples/example1.conf'), |
There was a problem hiding this comment.
Given its size, wouldn't it be clearer to use heredoc and inline it in the example?
There was a problem hiding this comment.
This is pretty much useless, because we already have it at manifests/vhosts.pp and REFERENCE.md for what it is worth.
There was a problem hiding this comment.
One pattern I've been using is to read examples in acceptance tests: https://github.com/theforeman/puppet-foreman_proxy/blob/d2f07f4a7e460231015f91469f298eb6831c7479/spec/spec_helper_acceptance.rb#L33-L40
It's on my agenda to add that example to voxpupuli-acceptance. The benefit is that you have syntax highlighting and linting for your code samples and you can browse the things we test.
So for now I wouldn't just get rid of it.
There was a problem hiding this comment.
There was a problem hiding this comment.
I would prefer the HEREDOC approach if only to get rid of the example data from the module under files/. Really that should only include what is actually used by the module. Given it is named examples though, it does seem very clear what is happening. I'll let @dhoppe flip a coin and decide to change it or not in a later PR :)
There was a problem hiding this comment.
@ekohl we do the same thing for sensu-puppet
Glad to hear I'm not alone in this. Given the code is very different, this clearly evolved separate which tells me there's a good chance it's a good idea :)
I would prefer the HEREDOC approach if only to get rid of the example data from the module under
files/
That was indeed my reason for it.
|
@ekohl could you please merge? |
|
Could I get a +1 on this refactor? |
|
IMO, it’s quite opinionated and would set a precedent to ditch the install, config, service subclass pattern. Not sure I want to go down this route given the current puppet documentation still recommends this approach. https://puppet.com/docs/puppet/7.5/bgtm.html#modules_ntp_example |
We said goodbye to params.pp and now to say goodbye to all these private classes that just break the code into multiple files without providing real value.
This is just copy/pasting the code back, except where noted in the comments.