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

Rotate Default Logs on Each 100MB #44888

Merged
merged 1 commit into from May 20, 2022

Conversation

berniechiu
Copy link
Contributor

@berniechiu berniechiu commented Apr 13, 2022

Summary

Also found that the default logs could grow quite big and left unnoticed, maybe we could set a default to 100MB which might already be enough for general usage and local development

Screen Shot 2022-04-13 at 11 31 02 PM

Other Information

REF: #44887

@rails-bot rails-bot bot added the railties label Apr 13, 2022
@skipkayhil
Copy link
Member

Hey @berniechiu, thanks for the contribution! I agree this would be a good thing to do as I also just found some development.log files that were larger than I expected. Since this is changing functionality can you add an entry to the changelog in railties?

@berniechiu
Copy link
Contributor Author

Sure, no prob

@berniechiu berniechiu force-pushed the rotate-default-log-files branch 2 times, most recently from 98d4f99 to 25c76f0 Compare May 5, 2022 18:01
@berniechiu berniechiu force-pushed the rotate-default-log-files branch 3 times, most recently from d44b84e to 4d2be01 Compare May 15, 2022 13:02
@berniechiu berniechiu requested a review from byroot May 15, 2022 13:30
railties/lib/rails/application/bootstrap.rb Outdated Show resolved Hide resolved
railties/lib/rails/application/configuration.rb Outdated Show resolved Hide resolved
railties/lib/rails/application/configuration.rb Outdated Show resolved Hide resolved
railties/test/application/configuration_test.rb Outdated Show resolved Hide resolved
@berniechiu berniechiu force-pushed the rotate-default-log-files branch 4 times, most recently from 0e89bf4 to 5c63b9c Compare May 16, 2022 13:56
@berniechiu berniechiu requested a review from byroot May 16, 2022 14:27
Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now.

I however forgot to mention that any new config needs an entry in https://github.com/rails/rails/blob/main/guides/source/configuring.md

railties/lib/rails/application/bootstrap.rb Outdated Show resolved Hide resolved
@berniechiu
Copy link
Contributor Author

I however forgot to mention that any new config needs an entry in https://github.com/rails/rails/blob/main/guides/source/configuring.md

Added, thanks for review

Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

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

Two minor things, after that I'm good.

guides/source/configuring.md Outdated Show resolved Hide resolved
railties/lib/rails/application/configuration.rb Outdated Show resolved Hide resolved
@berniechiu
Copy link
Contributor Author

berniechiu commented May 18, 2022

Re-request review button seems to be broken. But the comments are resolved by now

@byroot
Copy link
Member

byroot commented May 19, 2022

All good for me, unfortunately there is a conflict in the changelog. Ping me once you rebased, and I’ll merge

@berniechiu
Copy link
Contributor Author

berniechiu commented May 20, 2022

@eugeneius @byroot should be good by now, but looks like some flaky test failed in the spec? Since there's only text change in the markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants