jhoblitt / puppet-smartd

Manages the smartmontools package including the smartd daemon
Other
14 stars 24 forks source link

Format template to have spaces and linefeeds in the right places. #2

Closed razorsedge closed 11 years ago

razorsedge commented 11 years ago
class { 'smartd': 
  devices     => [ '/dev/sg1', '/dev/sg2', ],
  device_opts => {
    '/dev/sg1' => '-o on -S on -a',
    '/dev/sg2' => '-o on -S on -a',
  },
}

Before fix:

# Managed by Puppet -- do not edit!
DEFAULT -m root -M daily
/dev/sg1-o on -S on -a/dev/sg2-o on -S on -aDEVICESCAN

After fix:

# Managed by Puppet -- do not edit!
DEFAULT -m root -M daily
/dev/sg1 -o on -S on -a
/dev/sg2 -o on -S on -a
DEVICESCAN
jhoblitt commented 11 years ago

This looks good. Thank you for the contribution!

Are you willing to take a stab at adding a test case for this?

razorsedge commented 11 years ago

I would be more than happy to add test cases.

Do you have any plans to release this as a module, or perhaps push changes upstream to CSAIL?

jhoblitt commented 11 years ago

I exchanged several emails with the original author ~3 months ago when I forked the module for my local needs. I was told that in principle CSAIL was willing to pull patches from me. However, I see that there is recent activity in their repo and they have independently fixed some of the same issues I ran across. I think at this point I should probably go ahead and publish it on the forge as the divergence isn't completely trivial anymore, what do you think?

One thing I'm debating is moving the megaraid facts out of this module and into https://github.com/jhoblitt/puppet-megaraid_sm , which I really should rename to just megaraid. Input on that decision would be appreciated.

razorsedge commented 11 years ago

I would give another shot at trying to work with upstream. I'd hate to have two rather similar modules on the Forge even though you have added/improved all the documentation and unit tests.

Moving the megaraid facts out and into puppet-megaraid_sm seems to be a good idea. Actually, since I am evaluating use of puppet-megaraid_sm (and also puppet-lsi), I had one suggestion for puppet-megaraid_sm to either subclass or split out the MegaCLI installation. MSM does not seem to need it nor ship it in newer versions. With a separate megacli module, the megaraid facts could have a new home. puppet-smartd could require puppet-megacli and if the user enabled megacli installation ("include megacli" plus download and setup of a yumrepo), then the facts would appear.

As for renaming from megaraid_sm to megaraid, while it is shorter, MegaRAID is more of a brand name that refers to their hardware RAID cards. Maybe lsi_msm?

jhoblitt commented 11 years ago

https://github.com/jhoblitt/puppet-megaraid_sm is in need of some work and I have unpushed changes (somewhere... the joys of DVCS). I'll open a ticket on that module to discuss it. I will also plan on migrating the megacli dependent facts out of this module in the near future.

razorsedge commented 11 years ago

@jhoblitt How is this?

jhoblitt commented 11 years ago

This looks great! Thank you for the contribution and your tenacity to follow through with this.

Please note that I squashed your last commit so your repo will have conflicting commit ids.