search

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

Breaking: drop older Puppet version support, add data types, other breaking changes #108

Closed wyardley closed 6 years ago

wyardley commented 6 years ago

This is backwards incompatible in multiple ways

Additionally, we:

wyardley commented 6 years ago

I'm good with trying to rebase this if #111 gets merged, so we can definitely hold off until that at least.

sts commented 6 years ago

@wyardley After #113 you should be good to go with the additional refactoring and rebasing of this PR.

wyardley commented 6 years ago

I have rebased this, and the tests should all pass (with the one pending test from 20673eb8). I do wonder a bit if it's a little regressive to get rid of all the class params in favor of using config_hash for everything. We could reinstate some of them and build the defaults from those same values, which would allow for backwards compatibility, and allow for parameter validation.

@sts If it's not too hard, are you able to do some functional testing with this PR and make sure that it still works?

wyardley commented 6 years ago

@sts: Proposed some changes to make the CHANGELOG a little terser, let me know what you think. It's in a separate commit, so could easily get taken out or reverted. @craigwatson I don't know what your release process is, but I updated the CHANGELOG with my changes as well as reworked some of the stuff from #111. But there are probably other items that should go in there, the date might need to change, and entries might still need to be recategorized. I may or may not have time to test this this week with our setup, but hopefully some folks will be able to test this so that we can have more confidence.

wyardley commented 6 years ago

@craigwatson I rebased against your recent changes - think I got everything. I also updated params to match the versions of systems that are no longer supported (though I can revert that if you still want to allow those systems to work... I think at least CentOS 4 and Debian 4 are unlikely to need to be supported).

ps - I know that it may be hard to get acceptance tests working fully, since you'd need to also have Consul working, but I wonder if using spec_helper_acceptance might not be a bad way to add at least a basic acceptance test and provide a working Vagrant environment for testing stuff.

craigwatson commented 6 years ago

@wyardley thanks for the awesome work here - I think I'm going to put this on the back-burner for a few weeks as I'd like to get the module into a decent state before deprecating older Puppet versions.

The deprecation plan is (at the moment) to get one major release done to the Forge which is compatible with Puppet 3, and then drop in a deprecation message á la Voxpupuli - though I admit that this has been a long time coming!

Would you be happy to split the breaking changes out of this PR?

Edit - oops, didn't mean to close!

wyardley commented 6 years ago

@craigwatson I can take a look, but I think the whole PR is breaking, as data types are Puppet 4 only (and removal of validate_ can't happen before that), and using contain pattern over anchor pattern, and the switch to structured facts, will also not work in earlier Puppet versions. While switching to $foo from $::foo and removing the absolute classname check probably would be fine in this case, that was also a workaround related to Puppet 3. That really leaves only some minor changes (though you're welcome to cherry-pick out bits of the reworded changelog if it's helpful to you).

I can make a best effort to rebase again later if there are more changes, however, I'm moving to a job where I won't be working with Puppet and won't be using this module, so I may not have as much of an incentive to keep this updated. I could also try to break up the changes into commits that are easier to review separately, but that would be pretty hard to do now, especially if it ends up needing to be rebased later.

Puppet 3 is already EOL, and folks who need to support it could always get the older forge release and / or the Github fork. Vox has dropped Puppet 3 support in all modules quite a while back (and any modules that have a Puppet 3 branch are no longer maintained). So, while I absolutely agree that more functional testing before a Forge release was good, I'm not sure how necessary it is to do an explicit Forge release that supports Puppet 3 first.

One thing I didn't add in this PR, but should probably happen later, is to switch to Puppet strings for docs (and review that the class params documented are still the correct ones).

sts commented 6 years ago

@craigwatson release the current version to the forge and then lets proceed with @wyardley puppet 4 patch!

Please also get https://github.com/claranet/puppet-consul_template/pull/114 merged first, then we should be good to go for a last Puppet 3 release!

craigwatson commented 6 years ago

@sts you're too quick! I'm re-checking all dependencies at the moment, and will run a Forge release when done :)

craigwatson commented 6 years ago

@wyardley OK, now that we have a stable release to the Forge, we can merge these breaking changes and get version 2.0.0 released once we're happy. Can you rebase one last time? I've also incorporated your CHANGELOG alterations into the 1.0.0 release so it should make the PR code-only :)

wyardley commented 6 years ago

@craigwatson rebased again. btw, you left in the bit about requiring Puppet < 4.7.1 and stdlib >= 4.13 in the CHANGELOG for the last release. That isn't actually added until this is merged.

craigwatson commented 6 years ago

@wyardley thanks for all your help here - nice to finally get this merged!

A Forge release of 2.0.0 will be done soon 👍

wyardley commented 6 years ago

@craigwatson Thanks! Let me know if you need help with changelog or anything. BTW, since it's the same, might want to deprecate gdhbashton/consul_template in the forge, as it still shows up first in a search.