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

FEATURE: implement s3_canned_acl site setting #6271

Closed
wants to merge 3 commits into from

Conversation

@abelmokadem
Copy link

commented Aug 15, 2018

Implement S3 Canned ACL as a site setting

@discoursebot

This comment has been minimized.

Copy link

commented Aug 15, 2018

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

@abelmokadem abelmokadem changed the title FEATURE: implement s3_canned_acl site setting (WORK IN PROGRESS) FEATURE: implement s3_canned_acl site setting Aug 15, 2018
@discoursebot

This comment has been minimized.

Copy link

commented Aug 15, 2018

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

https://meta.discourse.org/t/migrating-uploads-to-s3-does-not-work-with-private-bucket/94591/5

Copy link
Member

left a comment

Code looks good 👍 but the tests need some attention.

@@ -250,6 +251,13 @@

end

it "s3_canned_acl" do

This comment has been minimized.

Copy link
@ZogStriP

ZogStriP Aug 20, 2018

Member

What's the point of this test? If it's testing the validation, then it should at least test for a bad case.

Also, the validation is already tested in s3_canned_acl_site_setting_spec.rb, so not sure what this is testing.

describe S3CannedACLSiteSetting do

describe 'valid_value?' do
it 'returns true for a valid S3 region' do

This comment has been minimized.

Copy link
@ZogStriP

ZogStriP Aug 20, 2018

Member

I don't think that's the right description ;)

@SamSaffron

This comment has been minimized.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

So sorry closing this, lets figure out another way of doing this from a plugin without adding a site setting.

@SamSaffron SamSaffron closed this Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.