FEATURE: Prohibit S3 bucket reusage (PR #6783)

This validation makes sure that the s3_upload_bucket and the s3_backup_bucket have different values. The backup bucket is allowed to be a subfolder of the upload bucket. The other way around is forbidden because the backup system searches by prefix and would return all files stored within the backup bucket and its subfolders.

GitHub

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

I usually prefer public_send to make sure we’re never calling a private method.

to_not raise_error should be deprecated in a later version if I am not wrong. Anyway you can read more about the problem with it here -> https://github.com/rspec/rspec-expectations/issues/231. If we’re not expecting an error, we can just call the method because an error would cause the test to fail anyway.

hmm I would just call this change_bucket_value. The word other isn’t really reflective of any actions being down in this method.

I didn’t read anything about deprecating not_to raise_error (without an error class). I think the more specific expect { }.not_to raise_error(SpecificErrorClass) doesn’t even work anymore. I like the to_not raise_error, because it’s more explicit to me as a reader, but I guess you are right and the “not raise an error” is already in the test name. Yeah, I’m going to remove it.

Oops! I forgot to push that change before I merged. :blush: Fixed in https://github.com/discourse/discourse/commit/2bdbca38012c4b46d8788a11369d968eb8abe885

I think the more specific expect { }.not_to raise_error(SpecificErrorClass) doesn’t even work anymore

OO it was for a specific error. I’ve been holding this thought in my head that not_to raise_error is deprecated.

@gschlager @tgxworld Prohibiting S3 bucket reuse should be enforced if not already done, was there ever any movement on this?

@0xtavian I’m not following. That’s exactly what this commit is all about.