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

Fixes #11413 - disable SQL logging by default #2617

Closed
wants to merge 1 commit into from

Conversation

tbrisker
Copy link
Member

No description provided.

@dLobatog
Copy link
Member

I think that if you want to change the default maybe config/logging.yaml is a better option. I'd recommend adding config/logging.yaml to the .gitignore and shipping a logging.yml.example as we do with settings and database. That way everyone can have their own option easily.

@tbrisker
Copy link
Member Author

@elobato I think this should be the default for rake, I've seen a couple of users already on IRC that missed the admin password because of this, and it also hides any errors when running migrations or tests. Note that if someone enables sql logging in the config it will still take precedence due to the reverse_merge, but if it isn't manually set I think the default should be off for rake and on for rails.

@domcleal
Copy link
Contributor

logging.yaml shouldn't really be used to enable/disable individual loggers though, that's what SETTINGS is for. I'd suggest just turning off SQL logging by default, it was done this way because it matched previous behaviour, but it ought to be off by default like other debug-level stuff (LDAP, permissions).

@dLobatog
Copy link
Member

Alright, set :sql => { :enabled => false } then and we close this off.

@domcleal
Copy link
Contributor

And update the settings.yaml.example to match, please.

@tbrisker tbrisker changed the title Fixes #11413 - disable SQL logging by default for rake Fixes #11413 - disable SQL logging by default Aug 20, 2015
@tbrisker
Copy link
Member Author

@elobato @domcleal I updated this as requested.

@domcleal
Copy link
Contributor

Yes, it has the re-review label already. There are a lot of PRs.

@domcleal
Copy link
Contributor

Merged as a683aa2, thanks @tbrisker.

@domcleal domcleal closed this Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants