Add toggle to control if Brakeman scans changed files, or all files #23
Conversation
Issues like unscoped finds are only detected if Brakeman scans the models, which means if a patch only changes a controller & introduces an unscoped find it won't be detected.
|
Not sure how to address the complexity issue raise by Code Climate, open to ideas! Also if there's a way to set this option from the config file, I'd much prefer to enable/disable the behaviour with that instead of a bare env var. |
|
Have you seen this comment? Until presidentbeef/brakeman#1368 is implemented, I think it would be better to always do this. While this would definitely slow up things for people running it as a git hook or where speed matters, we should have a flag for disabling it and it should clearly indicate what is happening e.g. |
|
Yes, that comment is why I made this change, since I saw it was impossible to do from Brakeman. I have opinions about changing the behaviour for existing users to make it slower, that's the kind of think I'd do if I could let them know about in advance. Also given the length of time that this behaviour has been the normal & how I'm the first person to suggest this, indicates there aren't many people affected by only scanning changed files. |
|
I've been bitten by this in the past, and hence we do a full brakeman run as part of CI checks (I've also seen this mentioned in various other places) -- it's just that people accept it as-is. On making it slower -- do you think if it'll be better if we bump up version to I'd rather we generated proper messages (a bit slower), especially since the upstream tool is so opinionated about the option we're using. Missing something invisibly is worse in my opinion than slowing up interactive/time-critical usage (which will be noted and investigated almost immediately). |
|
Any updates here? We have same problem but with
|
|
I think we need to make a call on whether this behavior is enabled by default or not. I'm in favor of doing it so, but the PR author (@dgholz) didn't reply to my comment. In the interim, |
Thanks for writing Pronto! We've integrated it into our CI pipeline, but have noticed that some checks fail to pick up some Brakeman issues, since Brakeman is passed a list of files to scan but checks like UnscopedFinds need scan data from models to detect problems. So if a change introduces an unscoped find in a controller,
pronto-brakemandoesn't pick it up.We'd rather have
pronto-brakemantake a bit longer to scan all the files when running in the CI pipeline, so I've added a check for an environmental variablePRONTO_BRAKEMAN_ONLY_FILESthat we can set to0in CI to force Brakeman to scan all files.