Merge branch 'rp/fix-import-sources' into 'master'

Fix data in import_sources column of application_settings

Closes gitlab-com/gl-infra/production#9134

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118757



Merged-by: Adam Hegyi <ahegyi@gitlab.com>
Approved-by: Thong Kuah <tkuah@gitlab.com>
Approved-by: Adam Hegyi <ahegyi@gitlab.com>
Co-authored-by: Reuben Pereira <rpereira@gitlab.com>

db/migrate/20230419105225_remove_phabricator_from_application_settings.rb

@@ -9,11 +9,11 @@ class ApplicationSetting < MigrationRecord
   end
 
   def up
-    # rubocop: disable Style/GuardClause
-    unless import_sources.empty?
-      ApplicationSetting.update_all(import_sources: import_sources.reject { |x| x == "phabricator" })
-    end
-    # rubocop: enable Style/GuardClause
+    return if import_sources.empty?
+
+    new_sources = import_sources - ['phabricator']
+
+    ApplicationSetting.update_all(import_sources: new_sources.to_yaml)
   end
 
   def down

db/migrate/20230426102200_fix_import_sources_on_application_settings_after_phabricator_removal.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class FixImportSourcesOnApplicationSettingsAfterPhabricatorRemoval < Gitlab::Database::Migration[2.1]
+  disable_ddl_transaction!
+
+  restrict_gitlab_migration gitlab_schema: :gitlab_main
+
+  class ApplicationSetting < MigrationRecord
+  end
+
+  def up
+    sources = ApplicationSetting.last&.import_sources
+
+    return unless sources.is_a?(String)
+    return if sources.start_with?('---')
+
+    sources = YAML.safe_load(sources)
+
+    ApplicationSetting.update_all(import_sources: sources.to_yaml)
+  end
+
+  def down; end
+end

db/schema_migrations/20230426102200

+81b99d212b01381866430868137e2e6a4abcb525a2be59849b2f3edd62d595b2
\ No newline at end of file

spec/migrations/20230419105225_remove_phabricator_from_application_settings_spec.rb

@@ -10,11 +10,13 @@
 
   describe "#up" do
     it 'removes phabricator and preserves existing valid import sources' do
-      settings.create!(import_sources: import_sources_with_phabricator)
+      record = settings.create!(import_sources: import_sources_with_phabricator)
 
       migrate!
 
-      expect(YAML.safe_load(ApplicationSetting.last.import_sources)).to eq(import_sources_without_phabricator)
+      expect(record.reload.import_sources).to start_with('---')
+
+      expect(ApplicationSetting.last.import_sources).to eq(import_sources_without_phabricator)
     end
   end
 end

spec/migrations/20230426102200_fix_import_sources_on_application_settings_after_phabricator_removal_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe FixImportSourcesOnApplicationSettingsAfterPhabricatorRemoval, feature_category: :importers do
+  let(:settings) { table(:application_settings) }
+  let(:import_sources) { %w[github git bitbucket bitbucket_server] }
+
+  describe "#up" do
+    shared_examples 'fixes import_sources on application_settings' do
+      it 'ensures YAML is stored' do
+        record = settings.create!(import_sources: data)
+
+        migrate!
+
+        expect(record.reload.import_sources).to start_with('---')
+        expect(ApplicationSetting.last.import_sources).to eq(import_sources)
+      end
+    end
+
+    context 'when import_sources is a String' do
+      let(:data) { import_sources.to_s }
+
+      it_behaves_like 'fixes import_sources on application_settings'
+    end
+
+    context 'when import_sources is already YAML' do
+      let(:data) { import_sources.to_yaml }
+
+      it_behaves_like 'fixes import_sources on application_settings'
+    end
+  end
+end