From 75f4a145685fbc54baa5ad828e6538734d0ef54c Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Tue, 19 Nov 2024 23:51:53 +0100 Subject: [PATCH] FIX: migrations-tooling CLI didn't work anymore (#29777) The previous approach of splitting Thor commands into multiple files caused problems when the same method name was used in multiple commands. This also loads the Rails environment only for commands that need it. That makes the CLI boot faster for most commands or when the help should be shown. That's also why we can't use `Rails.root` in the CLI. --- migrations/bin/cli | 46 ++++++++++++++---- migrations/lib/cli/convert_command.rb | 70 +++++++++++++-------------- migrations/lib/cli/import_command.rb | 21 ++++---- migrations/lib/cli/upload_command.rb | 61 ++++++++++------------- migrations/lib/migrations.rb | 12 +++-- 5 files changed, 114 insertions(+), 96 deletions(-) diff --git a/migrations/bin/cli b/migrations/bin/cli index f34f76fafc5..2341a0caa25 100755 --- a/migrations/bin/cli +++ b/migrations/bin/cli @@ -1,23 +1,49 @@ #!/usr/bin/env ruby # frozen_string_literal: true -require "thor" require_relative "../lib/migrations" +require "colored2" +require "thor" + module Migrations - load_rails_environment configure_zeitwerk enable_i18n - class CommandLineInterface < Thor - include ::Migrations::CLI::ConvertCommand - include ::Migrations::CLI::ImportCommand - include ::Migrations::CLI::UploadCommand + module CLI + class Application < Thor + desc "convert [FROM]", "Convert a file" + option :settings, type: :string, desc: "Path of settings file", banner: "path" + option :reset, type: :boolean, desc: "Reset database before converting data" + def convert(converter_type) + ::Migrations::CLI::ConvertCommand.new(converter_type, options).execute + end - def self.exit_on_failure? - true + desc "import", "Import a file" + def import + ::Migrations::CLI::ImportCommand.new(options).execute + end + + desc "upload", "Upload media uploads" + option :settings, + type: :string, + desc: "Uploads settings file path", + default: "./migrations/config/upload.yml", + aliases: "-s", + banner: "path" + option :fix_missing, type: :boolean, desc: "Fix missing uploads" + option :optimize, type: :boolean, desc: "Optimize uploads" + def upload + ::Migrations::CLI::UploadCommand.new(options).execute + end + + def self.exit_on_failure? + true + end end end - - Dir.chdir(Rails.root) { CommandLineInterface.start } # rubocop:disable Discourse/NoChdir end + +# rubocop:disable Discourse/NoChdir +Dir.chdir(File.expand_path("../..", __dir__)) { ::Migrations::CLI::Application.start } +# rubocop:enable Discourse/NoChdir diff --git a/migrations/lib/cli/convert_command.rb b/migrations/lib/cli/convert_command.rb index 258f6d417a3..ab11e7caf42 100644 --- a/migrations/lib/cli/convert_command.rb +++ b/migrations/lib/cli/convert_command.rb @@ -1,52 +1,48 @@ # frozen_string_literal: true -module Migrations::CLI::ConvertCommand - def self.included(thor) - thor.class_eval do - desc "convert [FROM]", "Convert a file" - option :settings, type: :string, desc: "Path of settings file", banner: "path" - option :reset, type: :boolean, desc: "Reset database before converting data" - def convert(converter_type) - converter_type = converter_type.downcase - validate_converter_type!(converter_type) +module Migrations::CLI + class ConvertCommand + def initialize(converter_type, options) + @converter_type = converter_type.downcase + @options = options + end - settings = load_settings(converter_type) + def execute + validate_converter_type! + settings = load_settings - ::Migrations::Database.reset!(settings[:intermediate_db][:path]) if options[:reset] + ::Migrations::Database.reset!(settings[:intermediate_db][:path]) if @options[:reset] - converter = "migrations/converters/#{converter_type}/converter".camelize.constantize - converter.new(settings).run - end + converter = "migrations/converters/#{@converter_type}/converter".camelize.constantize + converter.new(settings).run + end - private + private - def validate_converter_type!(type) - converter_names = ::Migrations::Converters.names + def validate_converter_type! + converter_names = ::Migrations::Converters.names - raise Thor::Error, <<~MSG if !converter_names.include?(type) - Unknown converter name: #{type} - Valid names are: #{converter_names.join(", ")} - MSG - end + raise Thor::Error, <<~MSG if !converter_names.include?(@converter_type) + Unknown converter name: #{@converter_type} + Valid names are: #{converter_names.join(", ")} + MSG + end - def validate_settings_path!(settings_path) - if !File.exist?(settings_path) - raise Thor::Error, "Settings file not found: #{settings_path}" - end - end + def validate_settings_path!(settings_path) + raise Thor::Error, "Settings file not found: #{settings_path}" if !File.exist?(settings_path) + end - def load_settings(converter_type) - settings_path = calculate_settings_path(converter_type) - validate_settings_path!(settings_path) + def load_settings + settings_path = calculate_settings_path + validate_settings_path!(settings_path) - YAML.safe_load(File.read(settings_path), symbolize_names: true) - end + YAML.safe_load(File.read(settings_path), symbolize_names: true) + end - def calculate_settings_path(converter_type) - settings_path = - options[:settings] || ::Migrations::Converters.default_settings_path(converter_type) - File.expand_path(settings_path, Dir.pwd) - end + def calculate_settings_path + settings_path = + @options[:settings] || ::Migrations::Converters.default_settings_path(@converter_type) + File.expand_path(settings_path, Dir.pwd) end end end diff --git a/migrations/lib/cli/import_command.rb b/migrations/lib/cli/import_command.rb index 2709397f87f..5365bffc276 100644 --- a/migrations/lib/cli/import_command.rb +++ b/migrations/lib/cli/import_command.rb @@ -1,15 +1,18 @@ # frozen_string_literal: true -module Migrations::CLI::ImportCommand - def self.included(thor) - thor.class_eval do - desc "import", "Import a file" - def import - require "extralite" +require "extralite" - puts "Importing into Discourse #{Discourse::VERSION::STRING}" - puts "Extralite SQLite version: #{Extralite.sqlite3_version}" - end +module Migrations::CLI + class ImportCommand + def initialize(options) + @options = options + end + + def execute + ::Migrations.load_rails_environment + + puts "Importing into Discourse #{Discourse::VERSION::STRING}" + puts "Extralite SQLite version: #{Extralite.sqlite3_version}" end end end diff --git a/migrations/lib/cli/upload_command.rb b/migrations/lib/cli/upload_command.rb index 61210fef671..843cb1f70d5 100644 --- a/migrations/lib/cli/upload_command.rb +++ b/migrations/lib/cli/upload_command.rb @@ -1,49 +1,40 @@ # frozen_string_literal: true -module Migrations::CLI::UploadCommand - def self.included(thor) - thor.class_eval do - desc "upload", "Upload media uploads" - option :settings, - type: :string, - desc: "Uploads settings file path", - default: "./migrations/config/upload.yml", - aliases: "-s", - banner: "path" - option :fix_missing, type: :boolean, desc: "Fix missing uploads" - option :optimize, type: :boolean, desc: "Optimize uploads" - def upload - puts "Starting uploads..." +module Migrations::CLI + class UploadCommand + def initialize(options) + @options = options + end - validate_settings_file! - settings = load_settings + def execute + puts "Starting uploads..." - ::Migrations::Uploader::Uploads.perform!(settings) + validate_settings_file! + settings = load_settings - puts "" - end + ::Migrations::Uploader::Uploads.perform!(settings) - private + puts "" + end - def load_settings - settings = ::Migrations::SettingsParser.parse!(options.settings) - merge_settings_from_cli_args!(settings) + private - settings - end + def load_settings + settings = ::Migrations::SettingsParser.parse!(@options.settings) + merge_settings_from_cli_args!(@options, settings) - def merge_settings_from_cli_args!(settings) - settings[:fix_missing] = options.fix_missing if options.fix_missing.present? - settings[:create_optimized_images] = options.optimize if options.optimize.present? - end + settings + end - def validate_settings_file! - path = options.settings + def merge_settings_from_cli_args!(settings) + settings[:fix_missing] = options.fix_missing if @options.fix_missing.present? + settings[:create_optimized_images] = options.optimize if @options.optimize.present? + end - if !File.exist?(path) - raise ::Migrations::NoSettingsFound, "Settings file not found: #{path}" - end - end + def validate_settings_file! + path = @options.settings + + raise ::Migrations::NoSettingsFound, "Settings file not found: #{path}" if !File.exist?(path) end end end diff --git a/migrations/lib/migrations.rb b/migrations/lib/migrations.rb index cb51a43c4b8..ddd829d098e 100644 --- a/migrations/lib/migrations.rb +++ b/migrations/lib/migrations.rb @@ -19,7 +19,7 @@ module Migrations def self.load_rails_environment(quiet: false) message = "Loading Rails environment ..." - print message unless quiet + print message if !quiet rails_root = File.expand_path("../..", __dir__) # rubocop:disable Discourse/NoChdir @@ -33,9 +33,11 @@ module Migrations end # rubocop:enable Discourse/NoChdir - print "\r" - print " " * message.length - print "\r" + if !quiet + print "\r" + print " " * message.length + print "\r" + end end def self.configure_zeitwerk @@ -49,7 +51,7 @@ module Migrations loader.push_dir(File.join(::Migrations.root_path, "lib"), namespace: ::Migrations) loader.push_dir(File.join(::Migrations.root_path, "lib", "common"), namespace: ::Migrations) - # All sub-directories of a converter should have the same namespace. + # All subdirectories of a converter should have the same namespace. # Unfortunately `loader.collapse` doesn't work recursively. Converters.all.each do |name, converter_path| module_name = name.camelize.to_sym