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
MONGOID-5304: Add Rubocop static code analyzer #284
MONGOID-5304: Add Rubocop static code analyzer #284
Conversation
Ready to merge, please check Evergreen setup works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @johnnyshields ,
As per https://jira.mongodb.org/browse/MONGOID-5304, if you would like to adjust this PR to only enable non-style/formatting checks, we will review it.
@p-mongo this is updated as per your comments in https://jira.mongodb.org/browse/MONGOID-5304 |
This reverts commit 0f42395.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost working but in evergreen the Ruby installation is under our tree and is checked by rubocop, it needs to be reconfigured to only explicitly check lib
.
@p-mongo I can't see what Evergreen is doing, but you should be able to set:
|
I could, but I would like |
@p-mongo is this done? Please kindly merge and then I'll port this to Mongoid and Ruby Driver libs. |
No, the checks should only be done on |
@p-mongo I've updated to only include |
.rubocop.yml
Outdated
TargetRubyVersion: 2.5 | ||
NewCops: disable | ||
Include: | ||
- 'lib/**/*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be the top-level lib
only, not any of the lib
subdirs in the current directory:
[2022/06/27 02:13:57.329] Running tests
[2022/06/27 02:13:57.329] Inspecting 144 files
[2022/06/27 02:13:57.329] ....................................................................................C................................................C..........
[2022/06/27 02:13:57.329] Offenses:
[2022/06/27 02:13:57.329] rubies/ruby-3.1.0/lib/ruby/gems/3.1.0/gems/ruby-prof-1.4.3/bin/ruby-prof:279:9: C: Security/Eval: The use of eval is a serious security risk.
[2022/06/27 02:13:57.329] eval(exec)
[2022/06/27 02:13:57.329] ^^^^
[2022/06/27 02:13:57.329] rubies/ruby-3.1/lib/ruby/gems/3.1.0/gems/ruby-prof-1.4.3/bin/ruby-prof:279:9: C: Security/Eval: The use of eval is a serious security risk.
[2022/06/27 02:13:57.329] eval(exec)
[2022/06/27 02:13:57.329] ^^^^
[2022/06/27 02:13:57.329] 144 files inspected, 2 offenses detected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... this failure looks Evergreen specific. The syntax I'm using here Include: - lib/**/*
is standard as per docs and means "from project root".
You can see the Github actions build is passing here: https://github.com/mongodb/bson-ruby/runs/7051219018?check_suite_focus=true
Note in particular there are 43 files scanned in Github Actions, vs 144 in your log:
Run bundle exec rubocop --parallel -c .rubocop.yml
[bson] Warning: No private key present, creating unsigned gem.
Inspecting 43 files
...........................................
43 files inspected, no offenses detected
Since I can't access Evergreen, you're going to have to debug this. Some ideas:
- check Rubocop is run from project root (not system root).
- check that the ruby-prof gem isn't being copied into the scan dir as part of the build process.
- try
Include:
arg as/path/from/systemroot/bson-ruby/lib/**/*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the Ruby installation together with gems under the project root in evergreen.
The syntax I'm using here Include: - lib/**/* is standard as per docs and means "from project root".
lib/**/*
in shell means ./lib/**/*
. In Rubocop this apparently means ./**/lib/**/*
. After a quick look at the configuration page I couldn't find where Include
is described, but the configuration in this PR needs to match the shell behavior of the glob as I just described it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a shot in the dark, let's try /lib/**/*
instead of lib/**/*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GH actions passed. Please try running Evergreen and kindly let me know what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please find out whether /lib/**/*
actually traverses from filesystem root and report your findings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So /lib/**/*
won't work. :(
I've configured rubocop with the -L
flag which will dump out the list of files that it's seeing. Please run Evergreen on the current commit and kindly let me know the output, and that should help me further debug the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you need to run anything on evergreen. The requirement is that rubocop only checks the top-level lib
directory. So you should be able to create a local project that has:
lib/foo.rb
- with an eval
in it
bar/lib/foo.rb
- also with an eval
in it
and then work out a configuration which reports the error for the first file but not the second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@p-mongo I have done this already. Here's the evidence:
/workspace/rubocop-test$ ruby -e "puts Dir.glob('**/*')"
foo
foo/lib
foo/lib/test.rb
foo/lib/test1
foo/lib/test1/test.rb
lib
lib/test.rb
lib/test1
lib/test1/test.rb
# this is using the exact rubocop.yml in this PR
# the -L command outputs which files Rubocop is scanning
/workspace/rubocop-test$ rubocop -L
lib/test.rb
lib/test1/test.rb
# note there is also a /lib dir in my drive root with Ruby files in it.
Please kindly run Evergreen as a next step, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same line as it was before, no?
…son-ruby into add-static-code-analyzer
.rubocop.yml
Outdated
TargetRubyVersion: 2.5 | ||
NewCops: disable | ||
Include: | ||
- 'lib/**/*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same line as it was before, no?
…son-ruby into add-static-code-analyzer
.rubocop.yml
Outdated
NewCops: disable | ||
Exclude: | ||
# exclude everything except lib dir | ||
- !ruby/regexp /^(?!lib\/)/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creative. So what is this value documented to be exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sadly that Regexp didn't work. So I got more creative 😂
Check the latest, I'm 95% confident this will work on Evergreen as well.
752b506
to
495cd85
Compare
If there isn't a normal way to tell rubocop to only check files in top-level |
Yeah, agreed, I've raised rubocop/rubocop#10802 for this issue and added it to the code comment. I'll raise a follow-up PR if/when its fixed on their end. Suggest we merge this as-is, even though that line ain't pretty. FWIW, |
Closing; replaced by #303 |
This PR adds a static code analyzer (Rubocop) to the project, as required by MongoDB's Technical and Organizational Security Measures.
It is integrated with Github CI. To add it to Evergreen, simply run this shell command (only needs to be run once for the entire suite; not dependent on Ruby version, etc.) I've done a draft of Evergreen integration in this PR, but it may not work as I cannot run Evergreen myself.
Refer to the related Mongoid PR for further explanation.