Simplify setting default cops #965
Conversation
See https://docs.rubocop.org/rubocop/configuration.html#generic-configuration-parameters Confirmed no change in cop configuration ``` ./bin/cookstyle --show-cops | sha256sum ab51157cbb2627ade51ca9dd08e17602c038c1f8b2baf6b8e4b26c6b5ed4f7d6 - ``` Signed-off-by: David Crosby <dcrosby@fb.com>
In the course of updating the pinned RuboCop versions, there hadn't been changes to the enabled upstream defaults in chefstyle.yml, and so they've been silently broken. This becomes noticeable when running `rubocop -c config/chefstyle.yml`. Commenting out the cops keeps parity ``` ./bin/cookstyle --show-cops | sha256sum ab51157cbb2627ade51ca9dd08e17602c038c1f8b2baf6b8e4b26c6b5ed4f7d6 - ``` Signed-off-by: David Crosby <dcrosby@fb.com>
This was a slightly unusual one. FWICT rubocop-performance is used in the CI for Cookstyle, but isn't part of Chef Workstation. `--show-cops` doesn't show the Performance cops, since they're not loaded. This silent failure becomes an issue when we switch to RuboCop::ConfigLoader.configuration_from_file in a later patch. We may want to move these cops to the .rubocop.yml cookstyle project root (which explicitly requires rubocop-performance) in a separate patch. Still have `--show-cops` parity: ``` $ ./bin/cookstyle --show-cops | sha256sum ab51157cbb2627ade51ca9dd08e17602c038c1f8b2baf6b8e4b26c6b5ed4f7d6 - ``` Signed-off-by: David Crosby <dcrosby@fb.com>
|
@dafyddcrosby after your chefstyle PR I implemented a similar cookstyle change with Rubocop bumped and new cops enabled. Glad to see you fixed the annoying warnings about the chef version tas50@78c0346 |
Shamelessly snagged from chef#965 Signed-off-by: Tim Smith <tsmith84@gmail.com>
Nice! My original plan was to do the two simplification diffs, move chefstyle.yml and the lints in that repo into this one (since they're still the same RuboCop), migrate the chefstyle gem references to cookstyle, and then we could deprecate and archive chefstyle entirely. cc @tpowell-progress let's discuss this today in code review today, I'm thinking I'll address the current rubocop nits on this PR and I/we can rebase your commit onto this? |
…op vendoring upstream.yml Here's the interesting part of the PR stack. Instead of clobbering the RuboCop default constants, load config/default.yml into the default configuration with RuboCop::ConfigLoader#default_configuration= We don't need to keep vendoring upstream.yml, since we're pinning the RuboCop version anyhow. As seen with the earlier chefstyle.yml issues, the more effective way of handling new lints in RuboCop upgrades is using `diff` and `sha256sum` against `--show-cops`. As with before, this maintains `--show-cops` parity ``` $ ./bin/cookstyle --show-cops | sha256sum ab51157cbb2627ade51ca9dd08e17602c038c1f8b2baf6b8e4b26c6b5ed4f7d6 - ```
Changes the sha256 of `--show-cops` from ab51157cbb2627ade51ca9dd08e17602c038c1f8b2baf6b8e4b26c6b5ed4f7d6 to c41d1165a55c3f832b3533988f3d5cb26ab9f7f8f761e3c36ebb3a54b2b20c80 However, running `diff` against the output shows that it's cosmetic YAML changes - the `Enabled:` line has moved Signed-off-by: David Crosby <dcrosby@fb.com>
Now that we don't vendor the RuboCop files, we can drop the rake task and update the documentation. Signed-off-by: David Crosby <dcrosby@fb.com>
|
Kudos, SonarCloud Quality Gate passed!
|
Description
tl;dr Similar to chef/chefstyle#157, stop vendoring the RuboCop configurations directly.
I noticed a bunch of stale cop names in cookstyle.yml, and saw how manually intensive and error-prone the process is to update RuboCop in Cookstyle. Several of the cops were renamed and Cookstyle stopped checking for them - the current vendoring process didn't stop that. In https://github.com/facebook/chef-cookbooks we also take an allowlist-style approach to RuboCop lints, but use DisableByDefault to accomplish that. There's prior art here: chef/chefstyle#66 took a stab at this, but there were other changes mixed in besides that transition, and it doesn't look like there was a parity test of the configuration after the change. Exactly what prompted the revert wasn't written down (in the repo, at least).
This is an attempt at simplifying Cookstyle so that we don't need to vendor RuboCop configs. I put care into providing the step-by-step in each commit comment, including why several cops are now commented out in cookstyle.yml.
Related Issue
chef/chefstyle#157
Types of changes
Checklist:
Gemfile.lockhas changed, I have used--conservativeto do it and included the full output in the Description above.