claranet / puppet-consul_template

A Puppet module to manage the config and jobs of Consul Template from Hashicorp
Apache License 2.0
30 stars 89 forks source link

Main class should contain/anchor other classes #61

Closed JayH5 closed 8 years ago

JayH5 commented 8 years ago

Because the main class doesn't contain or anchor the classes it includes, we can't set dependencies on it. i.e. doing something like:

class { 'consul_template':
  version      => '0.12.2',
  config_dir   => '/etc/consul-template',
  consul_host  => '127.0.0.1',
  consul_port  => 8500,
  consul_retry => '10s',
  log_level    => 'warn',
  require      => Package['unzip']
}

...doesn't actually require that the unzip package is installed before Consul Template.

I can work on a PR for this, but for now I'm having to add Package['unzip'] -> Class['consul_template::install'] in my manifests as a workaround.

gdhbashton commented 8 years ago

Puppet never ceases to amaze me. Is it any wonder the cool kids rush to Ansible and Salt? A PR would be very kind. Reading through init.pp I expected that this would already be enough:

https://github.com/gdhbashton/puppet-consul_template/blob/master/manifests/init.pp#L127-L136

Oh hang on - look at that ~> on line 134 - can you tell me if you get better behaviour if that's a forceful -> ordering rather than a ~> subscription?

JayH5 commented 8 years ago

I don't think -> vs ~> will make much of a difference. From the docs: "~> (notification arrow) Causes the resource on the left to be applied first..."

I also haven't seen this in a Puppet module before. I'm not sure if you were aiming to do what I'm saying here with that?

Anyway... let me try draw up a PR.

gdhbashton commented 8 years ago

Let me know if that causes any grief - I'm hoping to push a new version to the Forge in the next day or two.