search

puppetlabs / puppetlabs-concat

File concatenation system for Puppet
Apache License 2.0
170 stars 303 forks source link

Hey PuppetLabs Module Team, you just broke 700 modules. Is fail in concat::setup really necessary? #89

Closed nanliu closed 11 years ago

nanliu commented 11 years ago

Ok, got your attention. I know you guys love metrics and I'll just point out you just broke a ton of modules on github with your latest change to the concat module.

https://github.com/search?l=puppet&p=82&q=%22include+concat%3A%3Asetup%22&ref=searchresults&type=Code https://github.com/puppetlabs/puppetlabs-concat/blob/master/manifests/setup.pp#L14

So the latest update for concat::setup fail function is a bit harsh:

if $caller_module_name != $module_name {
  fail("Use of private class ${name} by ${caller_module_name}")
}

What's the catastrophic issue that must halt the catalog compile if another module have included this class? (This was the pattern in the original concat module) Why not using warning instead which provides a path for migration?

This triggers a whole bunch of mandatory fixes, and honestly I don't see the value (it's a cleanup issue, not really a catalog compile halt). I wouldn't be screaming about this if it only affected a few people, and heck you guys still haven't fixed all your modules yet:

https://github.com/puppetlabs/puppetlabs-apache/blob/master/manifests/balancer.pp#L45 https://github.com/puppetlabs/puppetlabs-puppet/blob/master/manifests/master.pp#L86

So I'm going to end my rant here. Still love you guys, but think about the impact before you make these breaking changes.

ripienaar commented 11 years ago

+1, this is a bad idea

slippycheeze commented 11 years ago

+1 -- this also violates the idea that you can idempotently include a class multiple times without, y'know, harm. One of the core ideas that differentiate Puppet from procedural tools like Ruby and all. Which sucks.

jhoblitt commented 11 years ago

@nanliu I'm the bad guy here.

The motivation on this change was to prep the API for converting the module to use a type/provider instead of defines/classes. Ie, remove concat::setup completely. Needing to manually include concat::setup in the manifest was also undocumented . There are also other minor changes that have been made to the API with the intent that the next release will be a major version bump.

@daniel-pittman You can still include the class as many times are you like, just not from outside the module. This is the same pattern as is used in puppet-mcollective and it prevents classes from unintentional becoming a public entry point into the module, as has happened here.

Is it reasonable to convert the fail() into a warning() or should the change be backed out completely?

slippycheeze commented 11 years ago

@daniel-pittman You can still include the module as many times are you like, just not from outside the module. This is the same pattern as is used in puppet-mcollective and it prevents classes from unintentional becoming a public entry point into the module, as has happened here.

I think you are missing the point: this was not just "unintentionally becoming" a public entry point, it was an undocumented, but necessary, part of using the module. It was public API! This change may be good -- though why the heck you couldn't just include an empty stub concat::setup class when you moved to a type/provider is beyond me -- but breaking the undocumented but real requirement to include it from outside the class is absolutely not good.

javierbertoli commented 11 years ago

Joshua I was going to propose you just that: turn the fail into a deprecation warning and change the include concat::setup to something similar to this:

if !defined(Class[$concat::setup]) {
  include concat::setup
}

that way, you give advice that the API is going to change, provide the new functionallity while still provide backward compatibility?

slippycheeze commented 11 years ago

Joshua I was going to propose just that, to turn the fail into a deprecation warning, and change the include concat::setup to something similar to this:

if !defined(Class[$concat::setup]) { include concat::setup }

Just change it into literally "include concat::setup" because, y'know, "include" is idempotent and will work correctly regardless of the class being included previously or not. O_o

ripienaar commented 11 years ago

With Daniel here, there's nothing wrong with including this class from anywhere. Undocumented doesnt make it not an api, it just means I was lazy while maintaining the module.

But this module seem to have had a bunch of changes recently that has had very little thought about the actual users of it. Rather than complain that something is undocumented and then making it fail for everyone, just document how to use it and do not break it for 1000s of users.

javierbertoli commented 11 years ago

@daniel-pittman, you're right regarding just "include" :)

Also agree with him here

It was public API!

Being undocumented does not invalidate it as public API.

ripienaar commented 11 years ago

and fwiw this used to be documented but https://github.com/puppetlabs/puppetlabs-concat/commit/0ba071858db5c4e9c698fd2b67694ba63f40a076 removed which a subsequent commit then broke when it relied on a variable from the setup class in the parameters of the init.pp

totally legit use, it was broken and undocumented. Thats the problem here. People are using it as intended.

jhoblitt commented 11 years ago

@daniel-pittman Your comment was on idempotence, which is what I responded too. The guard that was inserted was not to prevent multiple inclusion in the manifest, as it's included repeatedly from else where in the module. I'm in no way claiming that concat::setup wasn't part of the de facto API.

@ripienaar This module has periodically suffered accidental breakage. The goal of the PRs I made were to add test coverage, input validation, and make some helpful API changes once before a major API bump -- all to improve stability. This is similar as to has been done recently with other modules. In addition to removing concat::setup from the API, the warn parameter was split into two values https://github.com/puppetlabs/puppetlabs-concat/commit/eaf8407941ddaade508de8f1b14b7115eb6761ca, the unused $gnu parameter to concat https://github.com/puppetlabs/puppetlabs-concat/commit/e4a53b6755c8169d0373caa212bcdf7c94ef1f4d, and the $backup param concat::fragment https://github.com/puppetlabs/puppetlabs-concat/commit/4d884d32426b085215121f148752e41e5e1e15f2 were removed.

So is the feeling here that the API should be immutable and all API changes backed out or that fail() instead of warning() on the inclusion of concat::setup was too aggressive?

ripienaar commented 11 years ago

Sure, a change that effects 700 modules negatively is too aggressive.

javierbertoli commented 11 years ago

@jhoblitt it seems the consensus is that fail() was too aggressive.

As a highly used module, I think that changes to the public API, whether documented or not, should have a deprecation warning period before, don't you agree?

nanliu commented 11 years ago

@jhoblitt, just because there's semver, does not mean we should wield it like a sword and make breaking changes lightly. Adding cool feature X or module pattern Y does not always mean API breaking changes. The rate of updates to PuppetLabs module is awesome, but I feel like breaking changes have been made without consideration to impact v.s. benefit.

Point and case, I made an ugly hack to the boxen project vagrant module, and the github guys thought the feature was cool but the implementation was crap, so they wrote a type and provider and wrapped it:

https://github.com/boxen/puppet-vagrant/blob/master/manifests/box.pp

My ugly code existed for a few days and they could just as easily removed it, but they gave me something better and let me kept my code without any changes.

You can build new types & providers without breaking things. Give people a migration path, don't cut us off, and we will follow and support you, otherwise these kind of change will fragment the user base. /rant mode off/

jhoblitt commented 11 years ago

I've just opened PR #90 that adds warning()s were possible on deprecated portions of the API. Is it sufficient to address all of the concerns captured in this issue?

apenney commented 11 years ago

OK, so having just caught up on this I have a few scattered points here:

1/ I support #90 and have added a comment about making the warning() more user friendly. 2/ I agree that we should have deprecated this before skipping to a fail() and that was partly my fault as I was also unaware concat::setup was part of the public API and thought this was a safe change. 3/ SemVer actually says you should deprecate in a blah.x.0 release before you change in a major and we should make sure that is followed properly in future. 4/ I'd like to remind everyone that we haven't released any of this code yet, it's still just sitting in master, so no modules were destroyed (yet) :)

On a more philosophical note I would really to find a way to get the word out that SemVer major versions are our way of honestly stating that a module has been seriously changed in a backwards non-compatible way (which is the SemVer definition of a major version) and that dependencies should always be against major versions, not just >= for this specific reason. We absolutely don't want to alienate the userbase (obviously, for one I'm the userbase and I don't want to hate myself), but we absolutely need to bring the state of module development to a point where it's much closer to traditional software and we're able to maintain point releases from previous major versions and release backwards incompatible changes in major versions without having to problem scads of compatibility code and attempts to jump through hoops to make sure nothing ever changes from an API point of view.

I am mostly just concerned that because in the past things never really changed people have become accustomed to low churn and that's caused people to do things like "rely on master" or "deploy every master commit directly to production" (we have a concat user who used to do that, until I broke his production environment several weeks ago).

We want master to be seen as the place for development, not the stable deployment point, which is the job of releases to the forge. In this case things went perfect, nanliu noticed we introduced a change that would have destroyed things and we are able to back it out before a release.

Where to go from here

nanliu commented 11 years ago

On Tue, Oct 29, 2013 at 5:51 PM, Ashley Penney notifications@github.comwrote:

OK, so having just caught up on this I have a few scattered points here:

1/ I support #90 https://github.com/puppetlabs/puppetlabs-concat/pull/90and have added a comment about making the warning() more user friendly. 2/ I agree that we should have deprecated this before skipping to a fail() and that was partly my fault as I was also unaware concat::setup was part of the public API and thought this was a safe change.

Sure, I'm fine with the revert. I don't want to dwell on this because it diverts from the bigger issue below.

3/ SemVer actually says you should deprecate in a blah.x.0 release before you change in a major and we should make sure that is followed properly in future.

I understand the need for breaking changes and how semver specifies this. But please give it some serious consideration when making a backwards incompatible change, and especially if it can be avoided. It's one thing if puppet agent needs to go from XML RPC to REST and you need major version change, we as end users set aside time to deal changes like puppet 2.7 to 3.x upgrades. Modules are different, it's a massive jigsaw puzzle of dependencies because they come from different sources, some from puppetlabs, some from users github modules, and some from internal. Also modules don't behave like libraries, I have no way to introspect versions and decide how to invoke concat so I can be compatible with multiple versions.

The way the current module team test modules is akin to bundle install .fixtures.yml, however your end users must deal with making all the modules compatible on the puppet master. I don't have a way for some modules to use concat 1.0, and other modules to use an incompatible concat 2.0. In your view, if a module require puppet-concat 2.0, semver done. As a consumer of modules, if concat 2.0 conflicts with my hodgepodge collection of modules, I either need to write my internal version of module X that doesn't require concat 2.0, or I'm going to set aside my day to fork and patch all those conflicting modules.

I really do appreciate the bug fixes, improved testing, and overall quality enhancements the module team has made over the past few month. But this attitude about backwards compatibility is making the adaption of these awesome modules harder and more brittle. If we had two non compatible versions of puppet-concat, and a different set of modules that used them, we've effectively splintered applications that can be deployed by puppet. I hope this illustrates why I think breaking changes in modules is fragmenting and weakening the module ecosystem, not making it better.

4/ I'd like to remind everyone that we haven't released any of this code yet, it's still just sitting in master, so no modules were destroyed (yet) :)

Maybe some enterprise customer is gating release with forge, but this is ignoring how people use github:

https://github.com/search?l=yaml&q=git%3A%2F%2Fgithub.com%2Fripienaar%2Fpuppet-concat.git&ref=searchresults&type=Code https://github.com/search?l=yaml&q=git%3A%2F%2Fgithub.com%2Fpuppetlabs%2Fpuppetlabs-concat.git&ref=searchresults&type=Code https://github.com/search?l=yaml&q=git%3A%2F%2Fgithub.com%2Fpuppetlabs%2Fpuppetlabs-stdlib.git&ref=cmdform&type=Code

If you search of .fixtures.yml on github, pinning version is the exception than the norm. People don't pin versions, they simply fork from upstream. I would argue master branch for modules should aim to adapt the same gold standard set by stdlib module, not the other way around.

On a more philosophical note I would really to find a way to get the word out that SemVer major versions are our way of honestly stating that a module has been seriously changed in a backwards non-compatible way (which is the SemVer definition of a major version) and that dependencies should always be against major versions, not just >= for this specific reason. We absolutely don't want to alienate the userbase (obviously, for one I'm the userbase and I don't want to hate myself), but we absolutely need to bring the state of module development to a point where it's much closer to traditional software and we're able to maintain point releases from previous major versions and release backwards incompatible changes in major versions without having to problem scads of compatibility code and attempts to jump through hoops to make sure nothing ever changes from an API point of view.

Sure, I don't want to tie your hands on innovating, but there better be a damn good reason something like concat need a new major semver.

I am mostly just concerned that because in the past things never really changed people have become accustomed to low churn and that's caused people to do things like "rely on master" or "deploy every master commit directly to production" (we have a concat user who used to do that, until I broke his production environment several weeks ago).

We want master to be seen as the place for development, not the stable deployment point, which is the job of releases to the forge. In this case things went perfect, nanliu noticed we introduced a change that would have destroyed things and we are able to back it out before a release.

Where to go from here

1/ We'll make sure we improve our processes around deprecation. 2/ I am going to talk with the forge team about some kind of hook for people on the module team to be able to easily look for things like "Who uses concat::setup" and other large searches across all the known public modules so we can not accidentally do this again. 3/ Figuring out what we can do to PMT, or .fixtures.yml, or whatever we need to do to make it easier for people to work within preferred major releases.

Have a repo and try to install all puppetlabs module together and see if they still deploy.

4/ We desperately need a way to communicate with the users of specific modules so we can run changes past them, nothing is worse than merging a PR after weeks of silence only to hear immediately that it broke things for people. In the past I pushed for a modules@ mailing list but we need -something- so that we can discuss potential changes with users.

So in summary, because puppet module does not support running multiple versions simultaneously, please don't treat bumping major semver versions lightly for these core modules.

apenney commented 11 years ago

I really wish we had a mechanism to support multiple versions of modules in order to handle this problem, as really the heart of the pain people feel is "when composing lots of modules it's a horrible situation and argh". It's something I agree we need to tackle, especially as people rely more and more on third party modules. It's something we need to address, it's easy for me to sit on the module team and make changes in isolation but not so easy when composing them. I don't even know what a mechanism to address this is, but we're in the situation that we have to make everything backwards compatible to handle two modules needing different versions, and that's clumsy too.

jhoblitt commented 11 years ago

I feel like there's been a lot of good points covered in this issue/thread, that imminent disaster has been averted, and the horse meat has been well tenderized -- so it's time to close this issue out. I really wish there was a better forum for this type of discussion; perhaps the -dev list? Thank you to everyone for putting their time and energy into this discussion.