Shopify / erb_lint

Lint your ERB or HTML files
MIT License
655 stars 121 forks source link

Cache ignores Rubocop version and `rubocop.yml` configuration. #299

Open joshuapinter opened 1 year ago

joshuapinter commented 1 year ago

We ran into this when we upgraded the version of rubocop gem and our local ERB Linting (using --cache) did not find any issues but our CI Linting (not using --cache) found 56 errors.

When we disabled --cache in our local environment, the same 56 errors appeared. If we also used --clear-cache and then re-enabled --cache, we found the same 56 errors as well.

Thinking about it, this happens because of two reasons:

  1. New :cop:s are introduced in an upgraded version of the rubocop gem.
  2. You change your .rubocop.yml or .erb-lint.yml configuration to enable or change existing :cop:s.

So I'm thinking this could be solved with two main changes:

  1. Store the version of the rubocop gem in the cache. If this is different, clear the cache before running.
  2. Store the hash or something of both the .rubocop.yml and .erb-lint.yml files. If either of these are different, clear the cache before running.

Related to https://github.com/Shopify/erb-lint/pull/268 so I would love @zachfeldman's input since he masterminded this great caching feature.

Let me know your thoughts and I'm happy to take a look myself.

Thanks for the great work!

zachfeldman commented 1 year ago

Sorry to hear that using the cache introduced this issue! Definitely wouldn't want it to mask any new cops like you described, that doesn't sound like the right behavior.

Yes, I'd recommend that if the version of rubocop updates, we update the key here to bust the cache https://github.com/zachfeldman/erb-lint/blob/e08285533ad14d9ffcfc17ae1475f7d416647452/lib/erb_lint/cache.rb#L79. I wonder if it would make sense to calculate some value based on the Gemfile.lock to see if it changed so that any dependency update triggers the cache key to change, or just for certain gems like rubocop so it doesn't update too often?

I probably wouldn't have time to work on something like this right now myself with current commitments but good luck with patching it!

joshuapinter commented 1 year ago

@zachfeldman Thanks for the reply! No worries, I'll take a look at adding this now that you've done the heavy lifting.

I'm thinking the cache gets reset under these 3 scenarios:

  1. rubocop version changes.
  2. .rubocop.yml config changes.
  3. .erb-lint.yml config changes.

I haven't decided how best to accomplish this but likely a hash/signature made up of all three of these and if any one of them changes, the cache gets busted or cleared.

I'll keep you posted on progress...

joshuapinter commented 1 year ago

Hey Guys, I forked and wrote in the addition of the Gemfile.lock and the .rubocop.yml files into the cache so that if either of those change, the cache will break.

I noticed you were already doing this with .erb-lint.yml so that's great, no need to make any changes there. I just followed a similar pattern.

I placed the reading of both of these files in the Cache#initialize method so it doesn't get run every time checksum is called - maybe a little more performant.

Besides that, I tested this out extensively in development by disabling the pruning and making various changes to see if the caches were hit or not. Everything seemed to work great.

I attempted writing specs but got a little lost with how to make it work correctly. I couldn't see any pattern laid down with how you handle when .erb-lint.yml config is changed so I left that for now. Open to comment and suggestion.

We're gonna use our fork in "production" to test it more thoroughly and ensure any config or Gemfile changes won't produce false negatives.