4.0.0 breaks SSL configuration, taints node configuration and uses bad practices (imho)

[chr4]

9d42dbf is a horrible commit!
I'm shocked this made it into a released version in a cookbook that is that widely used.

1. Though there was at least a warning in the CHANGELOG, I now need to set all current (and really messy: future!) attributes when using the stable pgdg version of postgres (9.4.) on Ubuntu Trusty. One day the maintainer might introduce a new attribute depending on the version, which might result in messing up installations. I found this pretty annoying.
2. Tainting my node attribtues with node.set should be considered not a good style in general (imho). Furthermore, setting the ssl_key_file and ssl_cert_file parameters using default['postgresql']['config']['ssl_key_file'] = 'mycert.key' is not possible anymore and broke my configuration. The only thing preventing an outage was that I set the config_change_notify setting to :reload. I changed the default to override to make it work again, but this is highly inconsistent to the other settings!

I'm not sure what lead to this change, and I'm sure there was a good intention behind it, but this is really bad style and should not be the case in a cookbook that is used on production servers on probably thousands of machines. Please consider reverting this.

[chr4]

It gets worse. Deployments to heterogenous environments are made impossible with this commit.
The only sane workaround I found so far is adding tons of additional attributes to your environment file, if you want to continue using a certian version for a certian environment.
This requires hardcoding directories and packages for the whole environment.

All my node configuration files are tainted with tons of useless attributes, most of them would break the whole installation if they aren't overridden in some other place.

I don't know how this came though QA :(
Again, please revert this change!

[chr4]

#323 (comment)

[chr4]

Also, when using postgresql::client it installs the default version of the client package alongside the desired version. I set node['postgresql']['client']['packages'] to [ "postgresql-client-9.4", "libpq-dev" ], and postgresql-client-9.3 gets appended to the hash during the chef-run.

[luckymike]

@chr4 Released code passes (at minimum) the test suites in the cookbook. As such, the failure to catch this issue is likely attributable to a missing or incomplete test case.

Since you have intimate awareness of the conditions that cause a failure, perhaps you could submit a PR with a test case we can work against to resolve this?

[slyness]

@chr4 Feel free to fork and contribute. I work on this cookbook as much as possible and I personally put in quite a lot of work before the new year but there are many things that take priority. I believe that a majority of the changes which start version 4.0.0 are for the good. And the release was reviewed and tested by a number of people to help merge the release. This release also closed many issues for many other people. I believe fully in falling forward not falling backward even if new releases have issues. If you feel like you have a good idea for a solution and want to contribute a PR then I would highly encourage it. To date I don't see that as the case for your contributions here.

The origination of this cookbook extends to the beginning days of Chef and was originally authored by many contributors. I will continue to improve the code for this cookbook and continue to move it forward and I hope the community will be kind and gracious and think about doing the same. But that being said.. I'm not able to focus significant blocks of time weekly just only on this code base. I made a big push at the end of the year to set a start pointing.. this is a full major release of version. There will be continued issues to attend to I have no doubt. You should pin your dependencies accordingly and if you don't want to use the 4.x release yet then don't.

So.. enjoy it, hate it, use it, contribute to it.. Otherwise, please be more careful with your words. Your current method of communicating feels not very constructive.

[chr4]

@slyness Thanks for the efforts you put into this cookbook! I'm maintaining a few cookbooks myself, and I know how much work a maintainer invests. This wasn't meant to diminish anyones work, and I apologize if someone felt offended by my words. This was not my intention. Even though there was of course frustration speaking (like a database being dropped - I think that a lot of the issues are actually due to decisions in Chef itself, not only in this cookbook) I feel like I described the issues pretty rationally.

I stand to my point though, that a major version should not contain issues of this magnitude when released as stable to use. Furthermore, all changes causing the mentioned issues were not stated in the changelog.

To make this issue more constructive, I'll try to come up with a few pull requests e.g. improving the test cases like @luckymike proposed.

[luckymike]

@chr4 I'm closing this, because it does not present a specific addressable issue, but I do want to make clear that we've registered these concerns. We also welcome any PRs for test-cases that make these easier to resolve.

I think it's important to clarify two things:

a major version should not contain issues of this magnitude when released as stable to use

Major version release imply breaking changes. Obviously some of these are unintentional, but the versioning here implies "proceed with caution" and "caveat emptor." There is no more appropriate version to use (given the versioning available in Chef) than x.0.0 to communicate the magnitude of the risks a new version presents.

I apologize if someone felt offended by my words

You should apologize for using offensive language, not pass the buck to the person whose work you attacked. "horrible," "bad," & "tainting" are subjective and baseless criticisms. I recognize that you've registered some substantive concerns, but honestly, they're tough to tease out in a mix of unappreciative criticism of someone else's work.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.