Closed jlvallelonga closed 1 month ago
I’ll leave comments inline on the PR, but just to comment on the overall direction. I think it’s absolutely right to implement this as a configurable option via feature flag, but keep the default to be postgresql. For anyone running the app just for themselves, postgres is simpler setup. This S3-like option is really just for larger deployments.
I think this is all good, but I'm working on running the seeds to verify. I was able to create some files on the main branch and then checkout this branch, see it breaking, and then migrate to see it working again. If we're ok with that then feel free to merge, but otherwise I'll try to have the seeds working before I sleep tonight or tomorrow
Hi Justin, I had a busy day today and didn’t get a chance to take a look, but I should be able to tomorrow AM before you start (I’m still in Europe).
On Thu, Jul 18, 2024 at 2:51 PM Justin Vallelonga @.***> wrote:
I think this is all good, but I'm working on running the seeds to verify. I was able to create some files on the main branch and then checkout this branch, see it breaking, and then migrate to see it working again. If we're ok with that then feel free to merge, but otherwise I'll try to have the seeds working before I sleep tonight or tomorrow
— Reply to this email directly, view it on GitHub https://github.com/AllYourBot/hostedgpt/pull/465#issuecomment-2236585273, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIR5JNY72U3T5WWUNXF4TZM7B4LAVCNFSM6AAAAABLAROCCCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZWGU4DKMRXGM . You are receiving this because you commented.Message ID: @.***>
@jlvallelonga I gave this one more read through before merging in. I didn't love the special-casing of this one feature being ENV only (and needing to explain that) so I figured out how to make Feature...
work within application.rb. You can see it in this commit, if you're curious: 8873ede
(#465)
While I was in there, I did a close read-through of all the optional features we cover within the README and I did copy editing of that to try and streamline it a bit. While doing this, I decided to remove any mention of the rails credentials file. We'll still support it, but I'll consider that an advanced thing for people who already know how rails credentials work. For everyone else, it's easiest and more standard to simply set ENV.
@jlvallelonga I gave this one more read through before merging in. I didn't love the special-casing of this one feature being ENV only (and needing to explain that) so I figured out how to make
Feature...
work within application.rb. You can see it in this commit, if you're curious:8873ede
(#465)While I was in there, I did a close read-through of all the optional features we cover within the README and I did copy editing of that to try and streamline it a bit. While doing this, I decided to remove any mention of the rails credentials file. We'll still support it, but I'll consider that an advanced thing for people who already know how rails credentials work. For everyone else, it's easiest and more standard to simply set ENV.
nice! definitely more clean that way
First week, first PR merged in 🍾 🎉 It’s always nice to get something full cycle. Now we’re off!
Adds Cloudflare R2 as option for storing files with Active Storage.
Task from here: https://github.com/AllYourBot/hostedgpt/wiki/Draft-project-plan
Starting this as an optional change, but happy to modify this to make it a requirement or change it to be the default if we want.