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

Thread safety rubocop plugin #3543

Merged
merged 10 commits into from
Dec 3, 2019
Merged

Thread safety rubocop plugin #3543

merged 10 commits into from
Dec 3, 2019

Conversation

omgitsbillryan
Copy link
Contributor

Description of change

Adds a thread safety rubocop plugin to our linter.

Testing done

Run the lint locally using the command to only look for thread safety issues.

rubocop -r rubocop-thread_safety --only ThreadSafety,Style/GlobalVars,Style/ClassVars,Style/MutableConstant

Applies to all PRs

  • Appropriate logging
  • Swagger docs have been updated, if applicable
  • Provide link to originating GitHub issue, or connected to it via ZenHub
  • Does not contain any sensitive information (i.e. PII/credentials/internal URLs/etc., in logging, hardcoded, or in specs)
  • Provide which alerts would indicate a problem with this functionality (if applicable)

@omgitsbillryan omgitsbillryan requested review from a team as code owners November 13, 2019 18:48
@johnpaulashenfelter
Copy link
Contributor

@omgitsbillryan just curious if we've had any problems from thread safety in the past that this would pick up

@annaswims
Copy link
Contributor

I like it! How about committing the results of rubocop --auto-gen-config so that we can start checking future PR's right away and address existing problems one-by-one?

Gemfile Outdated
@@ -4,6 +4,8 @@ source 'https://rubygems.org'

ruby '2.4.5'

gem 'rubocop-thread_safety'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be moved to the group :development, :test block?

@edmangimelli
Copy link
Contributor

I'm sad by how little this readme tells me
https://github.com/covermymeds/rubocop-thread_safety

@omgitsbillryan
Copy link
Contributor Author

The jenkins build passed 🎉

But it looks like code climate is unable to load the plugin 🤔

/usr/local/lib/ruby/site_ruby/2.4.0/rubygems/core_ext/kernel_require.rb:117:in `require': cannot load such file -- rubocop-thread_safety (LoadError)

@annaswims
Copy link
Contributor

ugh! So, codeclimate doesn't run the version of rubocop from the gemfile. It does it's own nonsense and rubocop-thread_safety isn't in their gemfile https://github.com/codeclimate/codeclimate-rubocop/blob/channel-rubocop-0.71/Gemfile

I could see ditching codeclimate for this.

@annaswims
Copy link
Contributor

different rubocop gem, similar issue codeclimate/codeclimate-rubocop#131

@omgitsbillryan
Copy link
Contributor Author

It might be possible to execute the thread safety cop in a separate Jenkins stage purely on the command line rather than include it as a plugin in our .rubocop.yml config. This would give us the best of both worlds - except that thread safety exceptions won't show up in our codeclimate report.

@va-vfs-bot va-vfs-bot temporarily deployed to rubocop-thread-saftey/master December 3, 2019 15:17 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to rubocop-thread-saftey/master December 3, 2019 15:25 Inactive
@johnpaulashenfelter
Copy link
Contributor

We already run rubocop on CI, so why are we bothering running it again on CodeClimate, especially when it adds the "keep versions in sync" issue.

Isn't rubocop, for our purposes on the platform, like specs -- pass/fail?

Copy link
Contributor

@jholton jholton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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 this pull request may close these issues.

6 participants