FEATURE: Add s3_inventory_bucket site setting
#27013
Conversation
github-actions
bot
added
the
i18n
This commit adds a new `s3_inventory_bucket` site setting which allows admins to configure the S3 bucket in which S3 Inventory files should be stored at. The default for `s3_inventory_bucket` is blank and when it is blank, the S3 Inventory files will be stored in the S3 bucket given by `s3_upload_bucket`.
tgxworld
force-pushed
the
support_alternate_bucket_s3_inventory
branch
from
5821614
to
f2633c9
Compare
|
|
||
| CSV_KEY_INDEX ||= 1 | ||
| CSV_ETAG_INDEX ||= 2 | ||
| INVENTORY_PREFIX ||= "inventory" | ||
| INVENTORY_VERSION ||= "1" | ||
| INVENTORY_LAG ||= 2.days | ||
|
|
||
| def initialize(s3_helper, type, preloaded_inventory_file: nil, preloaded_inventory_date: nil) |
There was a problem hiding this comment.
I removed s3_helper from the arguments accepted by the constructor because the only valid instances of S3Helper are those generated by this constructor. In other words, only Discourse.store.s3_helper or S3Helper.new(SiteSetting.s3_inventory_bucket) are valid.
| @@ -9,8 +9,7 @@ | |||
| SiteSetting.enable_s3_inventory = true | |||
|
|
|||
| store = FileStore::S3Store.new | |||
| @client = Aws::S3::Client.new(stub_responses: true) | |||
There was a problem hiding this comment.
There was no need to set stub_responses: true here. I removed it and the tests still pass.
| @@ -5,9 +5,7 @@ | |||
| require "file_store/s3_store" | |||
|
|
|||
| RSpec.describe "S3Inventory", type: :multisite do | |||
| let(:client) { Aws::S3::Client.new(stub_responses: true) } | |||
There was a problem hiding this comment.
This is redundant too.
| describe "s3 inventory configuration" do | ||
| let(:bucket_name) { "s3-upload-bucket" } | ||
| let(:subfolder_path) { "subfolder" } | ||
| before { SiteSetting.s3_upload_bucket = "#{bucket_name}/#{subfolder_path}" } | ||
|
|
||
| it "is formatted correctly for subfolders" do | ||
| s3_helper = S3Helper.new(SiteSetting.Upload.s3_upload_bucket.downcase, "", client: client) | ||
| config = S3Inventory.new(s3_helper, :upload).send(:inventory_configuration) | ||
|
|
||
| expect(config[:destination][:s3_bucket_destination][:bucket]).to eq( | ||
| "arn:aws:s3:::#{bucket_name}", | ||
| ) | ||
| expect(config[:destination][:s3_bucket_destination][:prefix]).to eq( | ||
| "#{subfolder_path}/inventory/1", | ||
| ) | ||
| expect(config[:id]).to eq("#{subfolder_path}-original") | ||
| expect(config[:schedule][:frequency]).to eq("Daily") | ||
| expect(config[:included_object_versions]).to eq("Current") | ||
| expect(config[:optional_fields]).to eq(["ETag"]) | ||
| expect(config[:filter][:prefix]).to eq(subfolder_path) |
There was a problem hiding this comment.
Previous test was testing a private method which I didn't like so I rewrote the tests to stub the outgoing request instead.
| def stub_client_responses! | ||
| raise "This method is only allowed to be used in the testing environment" if !Rails.env.test? | ||
| @s3_client = init_aws_s3_client(stub_responses: true) | ||
| end |
There was a problem hiding this comment.
Actually I think there is no need to do this here, you can do something like this in tests instead
client = Aws::S3::Client.new(stub_responses: true)
client.stub_responses(xyz)
Aws::S3::Client.stubs(:new).returns(client)
There was a problem hiding this comment.
The problem is that this unnecessarily exposes the internals of S3Helper and when a person is initializing Aws::S3::Client manually, they are not initializing it with the necessary options which S3Helper will initialize the client with.
|
Ah I see what you mean here. If we are changing the bucket, we need to remove the inventory config from the old bucket. I'll have to think about this to see what is possible. |
This commit adds a new
s3_inventory_bucketsite setting which allowsadmins to configure the S3 bucket in which S3 Inventory files should be stored at.
The default for
s3_inventory_bucketis blank and when it is blank, theS3 Inventory files will be stored in the S3 bucket given by
s3_upload_bucket.