Foodcritic / foodcritic

Lint tool for Chef cookbooks.
http://foodcritic.io
MIT License
408 stars 153 forks source link

Release 12.3.0 should have been a breaking change #726

Closed haidangwa closed 6 years ago

haidangwa commented 6 years ago

Three foodcritic rules were removed from this release: FC017, FC057, and FC059. They were removed as defaults for Chef 13 no longer required them. However, for those still on Chef 12, this impacted linting. Removing a feature should be considered a breaking change; thus, the release should have been versioned as 13.0.0, per semver. Yes, I know that Chef 12 is EOL for 4/18, but that still does not negate the need as a breaking change for this source code, IMHO.

coderanger commented 6 years ago

How exactly is removing a rule a breaking change? If anything, adding rules is more likely to break things as they are normally enabled by default :)

haidangwa commented 6 years ago

When we're expecting said rules to be enforced by Foodcritic rules and all of a sudden they don't anymore, it causes a problem if someone submits a change to our cookbooks that should have triggered a foodcritic violation. Especially with use_inline_resources in a provider, it changes the result of the provider if we don't have that code anymore (in Chef 12). Adding functionality, on the other hand, is not always a breaking change, as the new features could be written in such a way that don't affect existing features.

On Mon, Feb 5, 2018 at 2:40 AM, Noah Kantrowitz notifications@github.com wrote:

How exactly is removing a rule a breaking change? If anything, adding rules is more likely to break things as they are normally enabled by default :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Foodcritic/foodcritic/issues/726#issuecomment-363046485, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2A7fgyGbXuMjvZMxNK4iK0xkgdTxKaks5tRtqkgaJpZM4R4-PL .

coderanger commented 6 years ago

So basically you are saying that any feature you expect Foodcritic to have but it does not have would be a breaking change? Because its going to be an uphill battle to make that claim on a volunteer project.

haidangwa commented 6 years ago

The expectation is in versioning properly. A minor version bump is supposed to be a compatible change; hence, anyone with a gem dependency like "foodcritic, ~> 12.2" should have a reasonable expectation to not be negatively impacted until 13 is released, but at least there is no expectation of backwards compatibility when a major version bump is released.

On Mon, Feb 5, 2018, 4:36 PM Noah Kantrowitz notifications@github.com wrote:

So basically you are saying that any feature you expect Foodcritic to have but it does not have would be a breaking change? Because its going to be an uphill battle to make that claim on a volunteer project.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Foodcritic/foodcritic/issues/726#issuecomment-363269757, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2A7ZS5htvNxOjWEQvJIJrFA_zMFRfaks5tR56egaJpZM4R4-PL .

coderanger commented 6 years ago

Sure, my point is that the presence of absence of individual rules is not something we have so far considered to be a breaking change because adding rules will very often "break" on existing code but that is kind of the point, and removing existing rules will never break per se but may produce false negatives. But claiming a false negative as "breaking" is hard when Foodcritic is, and will always be, best-effort at catching things since static analysis is just an approximation of code quality checks.

coderanger commented 6 years ago

Basically you are mad that we created a false negative in your CI pipeline and I'm sorry, but that's just how it goes. If you are stuck on a nearly EOL'd version of Chef, you should be using the corresponding time-locked version of ChefDK to ensure you get a suite of compatible tools.

haidangwa commented 6 years ago

On the contrary. I'm not bringing any emotion into this discussion at all. I'm bringing up a valid point and one that is adhered to by many development practices. If there's no clear adherence to a well established standard, how trustworthy is foodcritic at all?

On Mon, Feb 5, 2018, 11:01 PM Noah Kantrowitz notifications@github.com wrote:

Closed #726 https://github.com/Foodcritic/foodcritic/issues/726.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Foodcritic/foodcritic/issues/726#event-1459472199, or mute the thread https://github.com/notifications/unsubscribe-auth/AB2A7Skhdz0WAgLJG8XlDSkW2tuzsslnks5tR_i3gaJpZM4R4-PL .

coderanger commented 6 years ago

Foodcritic is, and will remain, a tool that does a best-effort static analysis to find common problems. Unfortunately "best-effort" is a purely subjective term and in this case it's mostly Tim that gets to have that opinion in a way that counts.