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

Update module structure #100

Closed sts closed 6 years ago

sts commented 7 years ago

Adopt module structure to the Github internal fork. It has several advantages:

sts commented 6 years ago

@craigwatson any thoughts on the future of this module? I'd like to discuss this before investing any further work.

craigwatson commented 6 years ago

@sts Apologies for the delay - my knowledge of Consul itself is admittedly lacking, but I see no downside to adopting the GitHub fork's structure, as you mentioned above it allows for a much cleaner module implementation.

Would you mind creating a PR for getting the fork back in-sync with this repo?

wyardley commented 6 years ago

@sts @craigwatson I can try to help on trying to incorporate those changes, since we need those too. However, I think first, it might be a good idea to implement data types and change the class structure around, getting rid of the contain pattern - I'll submit a PR that you can take a look at. Because of the drift (even before any structural changes), it may be hard to directly cherry-pick the changes.

wyardley commented 6 years ago

@ross - it seems like this fork is maintained again, are you still using this at Github, and would you be able / willing to help reconcile some of the changes in the fork?

If not, I will see if @duffrecords can work on adding some features (like the config_hash / config_defaults) back in.

ross commented 6 years ago

Hi @wyardley our fork is still in use, though not actively in development since it was just updated to do the bits we needed and we otherwise hadn't needed anything more from it. I was sad when the original PR on the origin stalled. We'd definitely look to go back to master if this happened, but unfortunately I won't personally have the bandwidth to look into getting our changes back in.

sts commented 6 years ago

I prepared a branch with the github module style in my fork. https://github.com/sts/puppet-consul_template/tree/github_module_style

One thing I wasn't sure about yet, is the dependency introduced to the puppet-consul module for the consul_sorted_json function, which is used to render the hash to the configuration file. Shall we keep it or rather copy it to a consul_template_sorted_json?

/cc @wyardley @ross

sts commented 6 years ago

@craigwatson Please stop accepting merges for new configuration values, until this is resolved otherwise we need to keep rebasing that patches. People should maintain them in their own forks until we are sorted here.

craigwatson commented 6 years ago

@sts thanks very much for the PR, apologies for the mess in the mean-time, I'll hold all PRs until we get the fork mess sorted 👍

sts commented 6 years ago

@craigwatson I finished the PR today, please review. I tested the code on Debian already.

sts commented 6 years ago

@craigwatson If possible please still delay the forge release, we just discovered a bug with consul_template::watch::perms. Its being rendered as an integer value.

sts commented 6 years ago

As a workaround one can assign permissions without the leading zero:

consul_template::watch { "example_file"
  config_hash => {
    perms => '644'
  }
}
craigwatson commented 6 years ago

@sts I don't have any plans for a Forge release currently - given the amount of change introduced I'm planning on creating a Vagrant environment for the module to make testing a little easier :)

phaer commented 6 years ago

As the closed issues above indicate, the issue with perms is now fixed for people using v3.2.3 or later o puppet-consul. de9baa7216ef8ee6f5cdc32aed93d60f1fd7f303 of puppet-consul_template works for us too.

craigwatson commented 6 years ago

@phaer thanks for the confirmation :)

@sts @wyardley Now that 1.0.0 has been released to the Forge, I'll close this issue 🎉