DEV: Improve s3:ensure_cors_rules logging (#14832)

DEV: Improve s3:ensure_cors_rules logging (#14832)

diff --git a/lib/s3_cors_rulesets.rb b/lib/s3_cors_rulesets.rb
index d982d18..7178e1a 100644
--- a/lib/s3_cors_rulesets.rb
+++ b/lib/s3_cors_rulesets.rb
@@ -26,6 +26,10 @@ class S3CorsRulesets
     max_age_seconds: 3000
   }.freeze
 
+  RULE_STATUS_SKIPPED = "rules_skipped_from_settings"
+  RULE_STATUS_EXISTED = "rules_already_existed"
+  RULE_STATUS_APPLIED = "rules_applied"
+
   ##
   # Used by the s3:ensure_cors_rules rake task to make sure the
   # relevant CORS rules are applied to allow for direct uploads to
@@ -38,33 +42,33 @@ class S3CorsRulesets
     return if !SiteSetting.s3_install_cors_rule
     return if !(GlobalSetting.use_s3? || SiteSetting.enable_s3_uploads)
 
-    assets_rules_applied = false
-    backup_rules_applied = false
-    direct_upload_rules_applied = false
+    assets_rules_status = RULE_STATUS_SKIPPED
+    backup_rules_status = RULE_STATUS_SKIPPED
+    direct_upload_rules_status = RULE_STATUS_SKIPPED
 
     s3_helper = S3Helper.build_from_config(
       s3_client: s3_client, use_db_s3_config: use_db_s3_config
     )
     puts "Attempting to apply ASSETS S3 CORS ruleset in bucket #{s3_helper.s3_bucket_name}."
-    assets_rules_applied = s3_helper.ensure_cors!([S3CorsRulesets::ASSETS])
+    assets_rules_status = s3_helper.ensure_cors!([S3CorsRulesets::ASSETS]) ? RULE_STATUS_APPLIED : RULE_STATUS_EXISTED
 
     if SiteSetting.enable_backups? && SiteSetting.backup_location == BackupLocationSiteSetting::S3
       backup_s3_helper = S3Helper.build_from_config(
         s3_client: s3_client, use_db_s3_config: use_db_s3_config, for_backup: true
       )
       puts "Attempting to apply BACKUP_DIRECT_UPLOAD S3 CORS ruleset in bucket #{backup_s3_helper.s3_bucket_name}."
-      backup_rules_applied = backup_s3_helper.ensure_cors!([S3CorsRulesets::BACKUP_DIRECT_UPLOAD])
+      backup_rules_status = backup_s3_helper.ensure_cors!([S3CorsRulesets::BACKUP_DIRECT_UPLOAD]) ? RULE_STATUS_APPLIED : RULE_STATUS_EXISTED
     end
 
     if SiteSetting.enable_direct_s3_uploads
       puts "Attempting to apply DIRECT_UPLOAD S3 CORS ruleset in bucket #{s3_helper.s3_bucket_name}."
-      direct_upload_rules_applied = s3_helper.ensure_cors!([S3CorsRulesets::DIRECT_UPLOAD])
+      direct_upload_rules_status = s3_helper.ensure_cors!([S3CorsRulesets::DIRECT_UPLOAD]) ? RULE_STATUS_APPLIED : RULE_STATUS_EXISTED
     end
 
     {
-      assets_rules_applied: assets_rules_applied,
-      backup_rules_applied: backup_rules_applied,
-      direct_upload_rules_applied: direct_upload_rules_applied
+      assets_rules_status: assets_rules_status,
+      backup_rules_status: backup_rules_status,
+      direct_upload_rules_status: direct_upload_rules_status
     }
   end
 end
diff --git a/lib/tasks/s3.rake b/lib/tasks/s3.rake
index 65b738d..954ba60 100644
--- a/lib/tasks/s3.rake
+++ b/lib/tasks/s3.rake
@@ -177,21 +177,9 @@ task 's3:ensure_cors_rules' => :environment do
   puts "Installing CORS rules..."
   result = S3CorsRulesets.sync(use_db_s3_config: use_db_s3_config)
 
-  if result[:assets_rules_applied]
-    puts "Assets rules did not exist and were applied."
-  else
-    puts "Assets rules already existed."
-  end
-  if result[:backup_rules_applied]
-    puts "Backup rules did not exist and were applied."
-  else
-    puts "Backup rules already existed."
-  end
-  if result[:direct_upload_rules_applied]
-    puts "Direct upload rules did not exist and were applied."
-  else
-    puts "Direct upload rules already existed."
-  end
+  puts "Assets rules status: #{result[:assets_rules_status]}."
+  puts "Backup rules status: #{result[:backup_rules_status]}."
+  puts "Direct upload rules status: #{result[:direct_upload_rules_status]}."
 end
 
 task 's3:upload_assets' => [:environment, 's3:ensure_cors_rules'] do
diff --git a/spec/lib/s3_cors_rulesets_spec.rb b/spec/lib/s3_cors_rulesets_spec.rb
index cbcd955..92e280b 100644
--- a/spec/lib/s3_cors_rulesets_spec.rb
+++ b/spec/lib/s3_cors_rulesets_spec.rb
@@ -40,9 +40,9 @@ RSpec.describe S3CorsRulesets do
           }
         )
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(true)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
-        expect(result[:backup_rules_applied]).to eq(false)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
+        expect(result[:backup_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
       end
 
       it "does not apply the ASSETS rules if they already exist" do
@@ -51,9 +51,9 @@ RSpec.describe S3CorsRulesets do
         })
         client.expects(:put_bucket_cors).never
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(false)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
-        expect(result[:backup_rules_applied]).to eq(false)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
+        expect(result[:backup_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
       end
 
       it "applies the ASSETS rules and the BACKUP_DIRECT_UPLOAD rules if S3 backups are enabled" do
@@ -77,9 +77,9 @@ RSpec.describe S3CorsRulesets do
           }
         )
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(true)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
-        expect(result[:backup_rules_applied]).to eq(true)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
+        expect(result[:backup_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
       end
 
       it "applies the ASSETS rules and the DIRECT_UPLOAD rules when S3 direct uploads are enabled" do
@@ -103,9 +103,9 @@ RSpec.describe S3CorsRulesets do
           }
         )
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(true)
-        expect(result[:direct_upload_rules_applied]).to eq(true)
-        expect(result[:backup_rules_applied]).to eq(false)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
+        expect(result[:backup_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
       end
 
       it "does no changes if all the rules already exist" do
@@ -117,9 +117,9 @@ RSpec.describe S3CorsRulesets do
         })
         client.expects(:put_bucket_cors).never
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(false)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
-        expect(result[:backup_rules_applied]).to eq(false)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
+        expect(result[:backup_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
       end
 
       def setup_backups
@@ -148,9 +148,9 @@ RSpec.describe S3CorsRulesets do
           }
         )
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(true)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
-        expect(result[:backup_rules_applied]).to eq(false)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
+        expect(result[:backup_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
       end
 
       it "does not apply the ASSETS rules if they already exist" do
@@ -159,9 +159,9 @@ RSpec.describe S3CorsRulesets do
         })

[... diff too long, it was truncated ...]

GitHub sha: fc98d1edfab36955cc867fc07bc494ca363fddf7

This commit appears in #14832 which was approved by CvX. It was merged by martin.