-
Notifications
You must be signed in to change notification settings - Fork 220
Migration fixes #385
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
Migration fixes #385
Conversation
|
What was the motivation for removing the |
src/db/file.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be useful to run the database query anyway, and instead of switching on content = 'in-s3', switch on whether the row exists in the table? I know it makes no difference for the production service, but it feels more resilient for anyone who hosts their own version of it and transitions to S3 after being online for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to, as it does add some amount of overhead and to an extent makes the code more complex. move-to-s3 should be the preferred route for migrating local installations to S3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, i'm just thinking about the period between saving S3 credentials into the environment and the moment cratesfyi database move-to-s3 finishes. Any calls to the website in that time will fail because we won't be looking at the database for the files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. move-to-s3 is no longer an operation that can run in the background. Since it's likely to never be used again outside local installations though that seems fine -- and if in the future there is a desire to make that happen the code change isn't really that bad.
|
I primarily removed the static files handler since independent of using S3 or the DB, it seems odd to store some files on disk and some on S3. There should be no reason to differentiate, IMO; in particular I always get annoyed when standing up a new instance as the new files need to basically be copied from production which isn't great. I can re-add it back and configure it on the staging server if you'd like, but I'd rather move us towards solely using S3/DB storage. |
|
I would be more willing to ditch the |
8104568 to
e7b6626
Compare
|
I've dropped the commit removing static_files. I realized that prod can just keep those files in S3, as before, since we check the database/S3 prior to looking at static files. |
QuietMisdreavus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Thanks so much.
#381 will also need to be merged, and this is based on #384.