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

make -z the default #852

Closed
grosser opened this Issue Mar 31, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@grosser
Contributor

grosser commented Mar 31, 2016

any test framework returns 1 on failure ... brakeman should too
easy to forget when setting up and always having green builds -> super dangerous

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Mar 31, 2016

Owner

That would be a pretty big change that could break things for users. While it's nice if people treat Brakeman as a test suite, it's not really the same and not everyone relies on it that way.

Owner

presidentbeef commented Mar 31, 2016

That would be a pretty big change that could break things for users. While it's nice if people treat Brakeman as a test suite, it's not really the same and not everyone relies on it that way.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Mar 31, 2016

Contributor

What would be a usecase that does not need a false exit status ?
earmark this for v2.0 ?

Contributor

grosser commented Mar 31, 2016

What would be a usecase that does not need a false exit status ?
earmark this for v2.0 ?

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Mar 31, 2016

Owner

Any time a user is expecting the exit status to mean Brakeman broke instead of meaning a warning was found.

Owner

presidentbeef commented Mar 31, 2016

Any time a user is expecting the exit status to mean Brakeman broke instead of meaning a warning was found.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Mar 31, 2016

Contributor

It's not very unixy to expect that ... especially any auditing/test command exits with 1 when the test/audit failed ... so I'd think it's the better default ...

Imagining an opposite world: would you except a PR that removes the exit 1 status if it was the default ? I would not ...

Contributor

grosser commented Mar 31, 2016

It's not very unixy to expect that ... especially any auditing/test command exits with 1 when the test/audit failed ... so I'd think it's the better default ...

Imagining an opposite world: would you except a PR that removes the exit 1 status if it was the default ? I would not ...

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Apr 1, 2016

Owner

Changing the default is a problem, no matter what. I'm not sure it's worth it when it's been the same for five years. It would certainly require a major version change.

Owner

presidentbeef commented Apr 1, 2016

Changing the default is a problem, no matter what. I'm not sure it's worth it when it's been the same for five years. It would certainly require a major version change.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Apr 1, 2016

Contributor

I've seen brakeman used in 10+ projects and always with -z flag, so I'd expect the fallout to be minimal :)
But agreed that it should be done on major version increment.

Contributor

grosser commented Apr 1, 2016

I've seen brakeman used in 10+ projects and always with -z flag, so I'd expect the fallout to be minimal :)
But agreed that it should be done on major version increment.

@presidentbeef presidentbeef added the 4.0 label May 24, 2016

@connorshea

This comment has been minimized.

Show comment
Hide comment
@connorshea

connorshea Aug 12, 2016

I 100% agree that -z should be the default.

connorshea commented Aug 12, 2016

I 100% agree that -z should be the default.

@GermanDZ

This comment has been minimized.

Show comment
Hide comment
@GermanDZ

GermanDZ Aug 18, 2017

Something related to -z the :exit_on_warn: true on a config file works fine in my OSX but is ignored in Linux. In Linux I need to add always -z despite of the configuration in the config file.

GermanDZ commented Aug 18, 2017

Something related to -z the :exit_on_warn: true on a config file works fine in my OSX but is ignored in Linux. In Linux I need to add always -z despite of the configuration in the config file.

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Aug 24, 2017

Owner

@GermanDZ sorry that's a bug I introduced in 3.7.1. It will be fixed in the next release.

Edit: although I don't know why it would be different on different OSes unless you are running different versions of Brakeman.

Owner

presidentbeef commented Aug 24, 2017

@GermanDZ sorry that's a bug I introduced in 3.7.1. It will be fixed in the next release.

Edit: although I don't know why it would be different on different OSes unless you are running different versions of Brakeman.

Repository owner locked and limited conversation to collaborators Sep 25, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.