DEV: Improve s3:ensure_cors_rules logging (#14832)

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
+    assets_rules_status = RULE_STATUS_SKIPPED
-    backup_rules_applied = false
+    backup_rules_status = RULE_STATUS_SKIPPED
-    direct_upload_rules_applied = false
+    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,
+      assets_rules_status: assets_rules_status,
-      backup_rules_applied: backup_rules_applied,
+      backup_rules_status: backup_rules_status,
-      direct_upload_rules_applied: direct_upload_rules_applied
+      direct_upload_rules_status: direct_upload_rules_status
     }
   end
 end

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 status: #{result[:assets_rules_status]}."
-    puts "Assets rules did not exist and were applied."
+  puts "Backup rules status: #{result[:backup_rules_status]}."
-  else
+  puts "Direct upload rules status: #{result[:direct_upload_rules_status]}."
-    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
 end
 
 task 's3:upload_assets' => [:environment, 's3:ensure_cors_rules'] do

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[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
-        expect(result[:backup_rules_applied]).to eq(false)
+        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[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
-        expect(result[:backup_rules_applied]).to eq(false)
+        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[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
-        expect(result[:backup_rules_applied]).to eq(true)
+        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[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:direct_upload_rules_applied]).to eq(true)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:backup_rules_applied]).to eq(false)
+        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[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
-        expect(result[:backup_rules_applied]).to eq(false)
+        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[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
-        expect(result[:backup_rules_applied]).to eq(false)
+        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
         })
         client.expects(:put_bucket_cors).never
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(false)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_EXISTED)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
-        expect(result[:backup_rules_applied]).to eq(false)
+        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
@@ -187,9 +187,9 @@ RSpec.describe S3CorsRulesets do
           }
         )
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(true)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:direct_upload_rules_applied]).to eq(false)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
-        expect(result[:backup_rules_applied]).to eq(true)
+        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
@@ -213,9 +213,9 @@ RSpec.describe S3CorsRulesets do
           }
         )
         result = sync_rules
-        expect(result[:assets_rules_applied]).to eq(true)
+        expect(result[:assets_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:direct_upload_rules_applied]).to eq(true)
+        expect(result[:direct_upload_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_APPLIED)
-        expect(result[:backup_rules_applied]).to eq(false)
+        expect(result[:backup_rules_status]).to eq(S3CorsRulesets::RULE_STATUS_SKIPPED)
       end
     end
   end