S3 configuration expectations between global and site settings

dev
1

I have a discourse instances where, months after initially creating the instance, we enabled “S3” (in our case, digital ocean spaces) for a while, only via admin settings, and never with any DISCOURSE_ config options in the app.yml file. We never migrated everything to S3, but the bulk of our images were written while we had it enabled. Now we want to migrate everything off “S3” for a variety of reasons not important here.

(I know this sounds like a support post so far. But it’s not…)

In a recently-merged PR, I added a rake task uploads:batch_migrate_from_s3 as a trivial wrapper around uploads:migrate_from_s3 to be able to migrate batches as small as only one post worth of images, and ran it. At this point I discovered that upload:migrate_from_s3 which my task calls assumes that discourse.conf has S3 settings in it that aren’t available in the UI:

** Invoke uploads:batch_migrate_from_s3 (first_time)
** Invoke environment (first_time)
** Execute environment
** Execute uploads:batch_migrate_from_s3
Migrating uploads from S3 to local storage for 'default'...
rake aborted!
NoMethodError: undefined method `downcase' for nil:NilClass
/var/www/discourse/app/models/global_setting.rb:107:in `s3_bucket_name'
/var/www/discourse/app/models/site_setting.rb:157:in `absolute_base_url'
/var/www/discourse/lib/tasks/uploads.rake:138:in `migrate_from_s3'
/var/www/discourse/lib/tasks/uploads.rake:118:in `block in migrate_all_from_s3'

What is the best solution?

  1. It’s expected that S3 setup is always done through DISCOURSE_ environment variables in the app.yml file and written into config/discourse.conf while building the container, and I should do that and rebuild my app now. (I note that the environment variable input to rake doesn’t seem to set GlobalSettings here either; I’ve tried that too. Presumably, the fact that anything worked with only SiteSettings configured is an oversight and unintended.) I tested this by putting s3_bucket into my config/discourse.conf in my existing container and it resolved the error for me.
  2. Modify the migrate task further so that if SiteSetting.s3_upload_bucket is set but GlobalSetting.s3_bucket is nil, set GlobalSetting.s3_bucket to SiteSetting.s3_upload_bucket — I’m guessing this wouldn’t be a hard PR if it’s considered right, but I haven’t looked hard yet. Edit: I tried modifying GlobalSetting.s3_bucket in uploads:migrate_from_s3 and that didn’t work because of lack of an accessor.
  3. Make SiteSetting.absolute_base_url use SiteSetting.s3_upload_bucket if GlobalSetting.s3_bucket is nil — is this even intended to be a right configuration?
  4. Add the ability to set s3_bucket in site settings so that it’s exposed in the UI.

The first is just site configuration, and I’ll probably just do it. If the second is considered a good alternative I’m happy to take a crack at PRing it. However, I’m new enough here that I’m not confident in making changes as core as the latter two; the expectations for how the config objects work are a bit opaque to me.

To be clear: I’ve worked trivially around my problem; it just surprised me that it was necessary.

2 Likes