From acd3f7c4e7e8bb5ebedb6a3c464da8d2118742e6 Mon Sep 17 00:00:00 2001 From: Ewoud Kohl van Wijngaarden Date: Wed, 22 Nov 2023 16:39:49 +0100 Subject: [PATCH] Fix some RuboCop rules (#848) * Fix Rails/FindEach cop find_each loads records in batches of 1000, which means it consumes less memory than loading everything at once. * Fix Rails/TimeZoneAssignment cop Setting the time zone as a block is now built into the library, so the custom implementation is no longer needed. * Fix Minitest/TestFileName cop Test files need to have the _test suffix, else they're not executed. * Add required_ruby_version to gemspec * Fix Style/SlicingWithRange cop In Ruby 2.6+ this is possible. --- .rubocop.yml | 2 +- .rubocop_todo.yml | 8 -------- ...51215114631_add_host_id_to_template_invocation.rb | 2 +- db/migrate/20151217092555_migrate_to_task_groups.rb | 2 +- ...20160113162007_expand_all_template_invocations.rb | 4 ++-- ...20160114125628_rename_job_name_to_job_category.rb | 4 ++-- ...ename_sudo_password_to_effective_user_password.rb | 4 ++-- .../20220321101835_rename_ssh_provider_to_script.rb | 2 +- extra/cockpit/foreman-cockpit-session | 2 +- foreman_remote_execution.gemspec | 2 ++ .../job_invocations/{create.rb => create_test.rb} | 0 test/unit/api_params_test.rb | 12 ++---------- ...r_scope_input.rb => renderer_scope_input_test.rb} | 0 13 files changed, 15 insertions(+), 29 deletions(-) rename test/graphql/mutations/job_invocations/{create.rb => create_test.rb} (100%) rename test/unit/{renderer_scope_input.rb => renderer_scope_input_test.rb} (100%) diff --git a/.rubocop.yml b/.rubocop.yml index d39c3eb83..51dcdca38 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -10,7 +10,7 @@ inherit_mode: - Exclude AllCops: - TargetRubyVersion: 2.5 + TargetRubyVersion: 2.7 TargetRailsVersion: 5.2 Exclude: - 'node_modules/**/*' diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 07f1f2fe9..f1b9bfc61 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -6,14 +6,6 @@ # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 2 -# Configuration parameters: Include. -# Include: **/*.gemspec -Gemspec/RequiredRubyVersion: - Exclude: - - 'foreman_remote_execution.gemspec' - - 'foreman_remote_execution_core.gemspec' - # Offense count: 1 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyleAlignWith, Severity. diff --git a/db/migrate/20151215114631_add_host_id_to_template_invocation.rb b/db/migrate/20151215114631_add_host_id_to_template_invocation.rb index 6a0d2dd5e..4757b0ffa 100644 --- a/db/migrate/20151215114631_add_host_id_to_template_invocation.rb +++ b/db/migrate/20151215114631_add_host_id_to_template_invocation.rb @@ -9,7 +9,7 @@ def up FakeTemplateInvocation.reset_column_information say 'Migrating existing execution locks to explicit relations, this may take a while' - FakeTemplateInvocation.all.each do |template_invocation| + FakeTemplateInvocation.all.find_each do |template_invocation| task = ForemanTasks::Task.for_action_types('Actions::RemoteExecution::RunHostJob').joins(:locks).where( :'foreman_tasks_locks.resource_type' => 'TemplateInvocation', :'foreman_tasks_locks.resource_id' => template_invocation.id diff --git a/db/migrate/20151217092555_migrate_to_task_groups.rb b/db/migrate/20151217092555_migrate_to_task_groups.rb index c542e507b..d603d2b42 100644 --- a/db/migrate/20151217092555_migrate_to_task_groups.rb +++ b/db/migrate/20151217092555_migrate_to_task_groups.rb @@ -5,7 +5,7 @@ class FakeJobInvocation < ApplicationRecord def up say 'Migrating from locks to task groups' - FakeJobInvocation.where('task_group_id IS NULL AND task_id IS NOT NULL').each do |job_invocation| + FakeJobInvocation.where('task_group_id IS NULL AND task_id IS NOT NULL').find_each do |job_invocation| task_group = JobInvocationTaskGroup.new task_group.task_ids = [job_invocation.task_id] task_group.save! diff --git a/db/migrate/20160113162007_expand_all_template_invocations.rb b/db/migrate/20160113162007_expand_all_template_invocations.rb index 9db69c32b..f0d8d9527 100644 --- a/db/migrate/20160113162007_expand_all_template_invocations.rb +++ b/db/migrate/20160113162007_expand_all_template_invocations.rb @@ -14,7 +14,7 @@ def up FakeTemplateInvocation.update_all 'host_id = NULL' # expand all pattern template invocations and link RunHostJob - JobInvocation.joins(:targeting).where("#{Targeting.table_name}.resolved_at IS NOT NULL").includes([:pattern_template_invocations, :targeting]).each do |job_invocation| + JobInvocation.joins(:targeting).where("#{Targeting.table_name}.resolved_at IS NOT NULL").includes([:pattern_template_invocations, :targeting]).find_each do |job_invocation| job_invocation.pattern_template_invocations.each do |pattern_template_invocation| job_invocation.targeting.hosts.each do |host| task = job_invocation.sub_tasks.find do |sub_task| @@ -38,7 +38,7 @@ def up def down # we can't determine the last host for pattern, but keeping template invocations as pattern is safe - JobInvocation.joins(:targeting).where("#{Targeting.table_name}.resolved_at IS NOT NULL").includes(:template_invocations).each do |job_invocation| + JobInvocation.joins(:targeting).where("#{Targeting.table_name}.resolved_at IS NOT NULL").includes(:template_invocations).find_each do |job_invocation| job_invocation.template_invocations.delete_all end end diff --git a/db/migrate/20160114125628_rename_job_name_to_job_category.rb b/db/migrate/20160114125628_rename_job_name_to_job_category.rb index ff8ad5d64..5c32a1173 100644 --- a/db/migrate/20160114125628_rename_job_name_to_job_category.rb +++ b/db/migrate/20160114125628_rename_job_name_to_job_category.rb @@ -3,13 +3,13 @@ def up rename_column :templates, :job_name, :job_category rename_column :job_invocations, :job_name, :job_category JobTemplate.where(:description_format => '%{job_name} %{command}').update_all(:description_format => 'Run %{command}') - JobTemplate.where("description_format LIKE '%\%{job_name}%'").each do |template| + JobTemplate.where("description_format LIKE '%\%{job_name}%'").find_each do |template| JobTemplate.where(:id => template.id).update_all(:description_format => template.description_format.gsub('%{job_name}', '%{job_category}')) end end def down - JobTemplate.where("description_format LIKE '%\%{job_category}%'").each do |template| + JobTemplate.where("description_format LIKE '%\%{job_category}%'").find_each do |template| JobTemplate.where(:id => template.id).update_all(:description_format => template.description_format.gsub('%{job_category}', '%{job_name}')) end JobTemplate.where(:description_format => 'Run %{command}').update_all(:description_format => '%{job_name} %{command}') diff --git a/db/migrate/20200623073022_rename_sudo_password_to_effective_user_password.rb b/db/migrate/20200623073022_rename_sudo_password_to_effective_user_password.rb index 48860eae1..821d094c5 100644 --- a/db/migrate/20200623073022_rename_sudo_password_to_effective_user_password.rb +++ b/db/migrate/20200623073022_rename_sudo_password_to_effective_user_password.rb @@ -2,7 +2,7 @@ class RenameSudoPasswordToEffectiveUserPassword < ActiveRecord::Migration[6.0] def up rename_column :job_invocations, :sudo_password, :effective_user_password - Parameter.where(name: 'remote_execution_sudo_password').each do |parameter| + Parameter.where(name: 'remote_execution_sudo_password').find_each do |parameter| record = Parameter.find_by(type: parameter.type, reference_id: parameter.reference_id, name: "remote_execution_effective_user_password") if record.nil? parameter.update(name: "remote_execution_effective_user_password") @@ -19,7 +19,7 @@ def up def down rename_column :job_invocations, :effective_user_password, :sudo_password - Parameter.where(name: 'remote_execution_effective_user_password').each do |parameter| + Parameter.where(name: 'remote_execution_effective_user_password').find_each do |parameter| record = Parameter.find_by(type: parameter.type, reference_id: parameter.reference_id, name: "remote_execution_sudo_password") if record.nil? parameter.update(name: "remote_execution_sudo_password") diff --git a/db/migrate/20220321101835_rename_ssh_provider_to_script.rb b/db/migrate/20220321101835_rename_ssh_provider_to_script.rb index 43977d1f6..0aec99d9b 100644 --- a/db/migrate/20220321101835_rename_ssh_provider_to_script.rb +++ b/db/migrate/20220321101835_rename_ssh_provider_to_script.rb @@ -6,7 +6,7 @@ def do_change(from, to, from_re, new_label) setting = Setting.find_by(:name => 'remote_execution_form_job_template') default_template = nil - Template.where(:provider_type => from).each do |t| + Template.where(:provider_type => from).find_each do |t| default_template = t if t.name == setting&.value t.provider_type = to t.name = t.name.gsub(from_re, new_label) diff --git a/extra/cockpit/foreman-cockpit-session b/extra/cockpit/foreman-cockpit-session index 3905c35c7..6211e6358 100755 --- a/extra/cockpit/foreman-cockpit-session +++ b/extra/cockpit/foreman-cockpit-session @@ -124,7 +124,7 @@ class ProxyBuffer def write_available! count = @dst_io.write_nonblock(@buffer) - @buffer = @buffer[count..-1] + @buffer = @buffer[count..] rescue IO::WaitWritable 0 rescue IO::WaitReadable diff --git a/foreman_remote_execution.gemspec b/foreman_remote_execution.gemspec index b812c49da..c205285b1 100644 --- a/foreman_remote_execution.gemspec +++ b/foreman_remote_execution.gemspec @@ -22,6 +22,8 @@ Gem::Specification.new do |s| s.test_files = `git ls-files test`.split("\n") s.extra_rdoc_files = Dir['README*', 'LICENSE'] + s.required_ruby_version = '>= 2.7', '< 4' + s.add_dependency 'deface' s.add_dependency 'dynflow', '>= 1.0.2', '< 2.0.0' s.add_dependency 'foreman-tasks', '>= 8.3.0' diff --git a/test/graphql/mutations/job_invocations/create.rb b/test/graphql/mutations/job_invocations/create_test.rb similarity index 100% rename from test/graphql/mutations/job_invocations/create.rb rename to test/graphql/mutations/job_invocations/create_test.rb diff --git a/test/unit/api_params_test.rb b/test/unit/api_params_test.rb index 4ac95e6e3..43cb9307d 100644 --- a/test/unit/api_params_test.rb +++ b/test/unit/api_params_test.rb @@ -11,23 +11,15 @@ class ApiParamsTest < ActiveSupport::TestCase end it 'honors explicitly supplied time zone' do - in_time_zone(ActiveSupport::TimeZone['America/New_York']) do + Time.use_zone(ActiveSupport::TimeZone['America/New_York']) do assert_equal '2022-07-08 08:53', params.send(:format_datetime, '2022-07-08 12:53:20 UTC') end end it 'implicitly honors current user\'s time zone' do - in_time_zone(ActiveSupport::TimeZone['America/New_York']) do + Time.use_zone(ActiveSupport::TimeZone['America/New_York']) do assert_equal '2022-07-08 12:53', params.send(:format_datetime, '2022-07-08 12:53:20') end end end - - def in_time_zone(zone) - old_tz = Time.zone - Time.zone = zone - yield - ensure - Time.zone = old_tz - end end diff --git a/test/unit/renderer_scope_input.rb b/test/unit/renderer_scope_input_test.rb similarity index 100% rename from test/unit/renderer_scope_input.rb rename to test/unit/renderer_scope_input_test.rb