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

Remove SQLite production warning but leave production config disabled #50463

Merged
merged 1 commit into from Dec 27, 2023

Conversation

byroot
Copy link
Member

@byroot byroot commented Dec 27, 2023

Fix: #49715

There are valid use cases for running SQLite in production, however it must be done with care, so instead of a warning most users won't see anyway, it's preferable to leave the configuration commented out to force them to think about having the database on a persistent volume etc.

Co-Authored-By: @intrip

production:
<<: *default
database: storage/production.sqlite3
# database: path/to/persistent/storage/production.sqlite3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# database: path/to/persistent/storage/production.sqlite3
# database: storage/production.sqlite3

I think just commenting it out is fine since most would probably use the default storage folder anyway, but persisted with something like Docker volumes.

@dhh
Copy link
Member

dhh commented Dec 27, 2023

This feels like a good compromise. But also wonder if we should support passing in a path using DATABASE_URL in production?

@byroot
Copy link
Member Author

byroot commented Dec 27, 2023

if we should support passing in a path using DATABASE_URL in production?

Makes sense to me but is orthogonal. Best handled in another PR.

@dhh
Copy link
Member

dhh commented Dec 27, 2023

Yeah I guess DATABASE_URL overwrites in any case. Looks good to me then 👍

@skipkayhil
Copy link
Member

Looks like rails-new-docker is correctly failing because it uses a sqlite3 db and now has no production configuration. Maybe it should be changed to one of the others?

@byroot
Copy link
Member Author

byroot commented Dec 27, 2023

I think I'll squeeze the DATABASE_URL support to fix it...

There are valid use cases for running SQLite in production, however it must be done
with care, so instead of a warning most users won't see anyway, it's preferable to
leave the configuration commented out to force them to think about having the database
on a persistent volume etc.

Co-Authored-By: Jacopo Beschi <beschi.jacopo@gmail.com>
@casperisfine casperisfine force-pushed the remove-sqlite3-production-warning branch from 11795be to 6b446be Compare December 27, 2023 22:32
@byroot
Copy link
Member Author

byroot commented Dec 27, 2023

Actually it's already supported.

@byroot byroot merged commit 354d1c4 into rails:main Dec 27, 2023
3 of 4 checks passed
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

4 participants