Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
joshuapinter opened this issue Dec 27, 2022 · 3 comments · May be fixed by #300
Open

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

joshuapinter opened this issue Dec 27, 2022 · 3 comments · May be fixed by #300

Comments

@joshuapinter
Copy link

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 👮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 👮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 #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
Copy link

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
Copy link
Author

@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 joshuapinter linked a pull request Jan 8, 2023 that will close this issue
3 tasks
@joshuapinter
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants