FIX: remove site setting 'shadowed-by-global' option (PR #8061)

GitHub

You’ve signed the CLA, ZogStriP. Thank you! This pull request is ready for review.

I support this, but I wonder, is there any situation where we have site settings for uploads that are fighting for names with global setting? we should do a quick audit for clashes here.

1 Like

Getting this merged would be insanely useful for a migration between instances I’m currently working on.

I’ve had a look at the s3 GlobalSettings and SiteSettings, which is what I assume you mean @sam:

Name GlobalSetting SiteSetting
s3_bucket (this is for assets)
s3_backup_bucket ✓ (shadowed)
s3_upload_bucket ✓ (shadowed)
s3_region ✓ (shadowed)
s3_access_key_id ✓ (shadowed)
s3_secret_access_key ✓ (shadowed)
s3_use_iam_profile ✓ (shadowed)
s3_cdn_url
s3_endpoint ✓ (shadowed)

So, since so many of these settings are already shadowed (and therefore already clash), the only use-case I see this breaking is if a user has their uploads and assets in s3, but behind different CDNs.

I’d suggest renaming the s3_cdn_url setting to something like s3_upload_cdn_url, and then I don’t think this PR would introduce any further clashes. It would also resolve the confusion seen here.

And, I don’t think that change would need to be made in many places, since there’s already some sanitation of these clashing settings happening here:

https://github.com/discourse/discourse/blob/7e69c5c/app/models/site_setting.rb#L142-L156

1 Like

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

What are we going to do here @ZogStriP / @eviltrout ?

I think we should merge it. It’s a good change.

I’d suggest renaming the s3_cdn_url setting to something like s3_upload_cdn_url, and then I don’t think this PR would introduce any further clashes. It would also resolve the confusion seen here.

@LeoMcA doesn’t look like that renaming was part of this PR…would you be willing to open a new one for that?

@SamSaffron @eviltrout could you weigh in on the renaming? It would appear there is currently no way to use DISCOURSE_S3_CDN_URL and not affect asset compilation.