From 33c5eeefdae6eb196c746334341178abf532912a Mon Sep 17 00:00:00 2001 From: Markus Bucher Date: Mon, 30 Oct 2023 17:07:34 +0100 Subject: [PATCH 1/2] Upgrade test-envs --- .github/workflows/ruby.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml index 81df192..af19e31 100644 --- a/.github/workflows/ruby.yml +++ b/.github/workflows/ruby.yml @@ -25,7 +25,7 @@ jobs: - name: Setup Ruby uses: ruby/setup-ruby@v1 with: - ruby-version: 2.6 + ruby-version: 2.7 bundler-cache: true - name: Run rubocop run: bundle exec rubocop @@ -44,7 +44,7 @@ jobs: fail-fast: false matrix: ruby-version: ['2.7'] - foreman-core-branch: ['develop', '3.4-stable', '3.3-stable'] + foreman-core-branch: ['develop', '3.7-stable', '3.5-stable'] proxmox: ['~>0.13.3'] # Steps represent a sequence of tasks that will be executed as part of the job From 9f645c3ea577444ff57fd7b3887f9038b74207e0 Mon Sep 17 00:00:00 2001 From: Markus Bucher Date: Mon, 30 Oct 2023 17:09:30 +0100 Subject: [PATCH 2/2] Update rubocop and use theforeman-rubocop gem --- .rubocop.yml | 58 +++++-------------- Gemfile | 5 +- Rakefile | 2 +- .../api/v2/snapshots_controller.rb | 9 +-- .../snapshots_controller.rb | 10 ++-- .../fog_extensions/proxmox/snapshots/mock.rb | 2 +- .../fog_extensions/vsphere/snapshots/mock.rb | 4 +- .../fog_extensions/vsphere/snapshots/real.rb | 2 +- .../proxmox_extensions.rb | 2 +- .../foreman_snapshot_management/snapshot.rb | 21 +++---- config/routes.rb | 2 +- lib/foreman_snapshot_management/engine.rb | 36 ++++++------ test/controllers/api/v2/snapshots_test.rb | 2 +- .../snapshots_controller_test.rb | 2 +- 14 files changed, 65 insertions(+), 92 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index f47f83f..f94eddf 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,17 +1,9 @@ -require: - - rubocop-performance - - rubocop-rails - - rubocop-minitest - -AllCops: - TargetRubyVersion: 2.5 - TargetRailsVersion: 5.2 - Exclude: - - 'locale/**/*' - - 'node_modules/**/*' - - 'vendor/**/*' - - '*.spec' +inherit_gem: + theforeman-rubocop: + #- lenient.yml + #- default.yml + - strict.yml Layout/LineLength: Max: 190 @@ -20,7 +12,9 @@ Metrics/AbcSize: Enabled: false Metrics/BlockLength: - Enabled: false + Max: 50 + Exclude: + - 'test/**/*.rb' Metrics/ClassLength: Max: 500 @@ -31,25 +25,8 @@ Metrics/MethodLength: Metrics/ModuleLength: Max: 400 -Metrics/ParameterLists: - Enabled: false - Metrics/PerceivedComplexity: - Enabled: false - -Naming/AccessorMethodName: - Enabled: false - -Naming/FileName: - Exclude: - - 'db/seeds.d/*' - -Naming/MethodName: - Exclude: - - 'app/models/concerns/orchestration/*.rb' - -Rails: - Enabled: True + Max: 10 Rails/Date: Exclude: @@ -69,32 +46,27 @@ Style/ClassAndModuleChildren: Style/Documentation: Enabled: false -Style/EachWithObject: - Enabled: false - Style/FormatString: - Enabled: false + EnforcedStyle: percent Style/FormatStringToken: - Enabled: false + EnforcedStyle: template Style/GuardClause: Enabled: false # Support both ruby19 and hash_rockets Style/HashSyntax: + EnforcedStyle: no_mixed_keys + +Style/OptionalBooleanParameter: Enabled: false - SupportedStyles: - - ruby19 - - hash_rockets Style/RedundantSelf: Enabled: false Style/RegexpLiteral: - Enabled: false - -Style/RescueModifier: + EnforcedStyle: slashes Enabled: false Style/StringLiterals: diff --git a/Gemfile b/Gemfile index 4a079e0..812b7ea 100644 --- a/Gemfile +++ b/Gemfile @@ -5,7 +5,4 @@ source 'https://rubygems.org' gemspec gem "rdoc", "~> 6.3.1" -gem "rubocop", "~> 0.89.0" -gem "rubocop-minitest", "~> 0.10.3" -gem "rubocop-performance", "~> 1.8" -gem "rubocop-rails", "~> 2.8" +gem "theforeman-rubocop", "~> 0.1.0" diff --git a/Rakefile b/Rakefile index 545e849..f66875f 100755 --- a/Rakefile +++ b/Rakefile @@ -18,7 +18,7 @@ begin require 'rubocop/rake_task' RuboCop::RakeTask.new rescue StandardError => _e - puts 'Rubocop not loaded.' + # 'Rubocop not loaded.' end RDoc::Task.new(:rdoc) do |rdoc| diff --git a/app/controllers/api/v2/snapshots_controller.rb b/app/controllers/api/v2/snapshots_controller.rb index 44bd503..547067f 100644 --- a/app/controllers/api/v2/snapshots_controller.rb +++ b/app/controllers/api/v2/snapshots_controller.rb @@ -16,7 +16,7 @@ class SnapshotsController < V2::BaseController meta :search => [{ :name => 'name', :type => 'string' }] def index if params[:search] - search = params[:search].match(/^\s*name\s*=\s*(\w+)\s*$/) || params[:search].match(/^\s*name\s*=\s*\"([^"]+)\"\s*$/) + search = params[:search].match(/^\s*name\s*=\s*(\w+)\s*$/) || params[:search].match(/^\s*name\s*=\s*"([^"]+)"\s*$/) raise "Field '#{params[:search]}' not recognized for searching!" unless search snapshot = resource_class.find_for_host_by_name(@host, search[1]) @@ -34,7 +34,8 @@ def index param :host_id, :identifier_dottable, :required => true param :id, :identifier_dottable, :required => true - def show; end + def show + end def_param_group :snapshot do param :snapshot, Hash, :required => true, :action_aware => true do @@ -51,8 +52,8 @@ def show; end def create @snapshot = resource_class.new(snapshot_params.to_h.merge(host: @host).merge( - include_ram: Foreman::Cast.to_bool(params[:include_ram]), quiesce: Foreman::Cast.to_bool(params[:quiesce]) - )) + include_ram: Foreman::Cast.to_bool(params[:include_ram]), quiesce: Foreman::Cast.to_bool(params[:quiesce]) + )) process_response @snapshot.create end diff --git a/app/controllers/foreman_snapshot_management/snapshots_controller.rb b/app/controllers/foreman_snapshot_management/snapshots_controller.rb index 5aaec7c..253f544 100644 --- a/app/controllers/foreman_snapshot_management/snapshots_controller.rb +++ b/app/controllers/foreman_snapshot_management/snapshots_controller.rb @@ -75,7 +75,8 @@ def update end define_action_permission ['select_multiple_host', 'create_multiple_host'], :create - def select_multiple_host; end + def select_multiple_host + end def create_multiple_host data = snapshot_params @@ -95,7 +96,7 @@ def create_multiple_host msg = _('Created %{snapshots} for %{num} %{hosts}') % { snapshots: n_('Snapshot', 'Snapshots', snapshots_created), num: snapshots_created, - hosts: n_('host', 'hosts', snapshots_created) + hosts: n_('host', 'hosts', snapshots_created), } # for backwards compatibility if respond_to? :success @@ -108,10 +109,11 @@ def create_multiple_host end def snapshot_mode_assignment(snapshot_mode, data) - if snapshot_mode == 'memory' + case snapshot_mode + when 'memory' data[:include_ram] = true data[:quiesce] = false - elsif snapshot_mode == 'quiesce' + when 'quiesce' data[:include_ram] = false data[:quiesce] = true else diff --git a/app/models/concerns/fog_extensions/proxmox/snapshots/mock.rb b/app/models/concerns/fog_extensions/proxmox/snapshots/mock.rb index 8aa7a49..082b57a 100644 --- a/app/models/concerns/fog_extensions/proxmox/snapshots/mock.rb +++ b/app/models/concerns/fog_extensions/proxmox/snapshots/mock.rb @@ -15,7 +15,7 @@ def status_task(_node, _upid) 'exitstatus' => 'OK', 'status' => 'stopped', 'id' => '100', - 'pid' => 15_891 + 'pid' => 15_891, } end end diff --git a/app/models/concerns/fog_extensions/vsphere/snapshots/mock.rb b/app/models/concerns/fog_extensions/vsphere/snapshots/mock.rb index 2ee10ae..cd1ccf1 100644 --- a/app/models/concerns/fog_extensions/vsphere/snapshots/mock.rb +++ b/app/models/concerns/fog_extensions/vsphere/snapshots/mock.rb @@ -15,7 +15,7 @@ def remove_snapshot(options = {}) raise ArgumentError, 'removeChildren is a required parameter' unless options.key? 'removeChildren' { - 'task_state' => 'success' + 'task_state' => 'success', } end @@ -25,7 +25,7 @@ def rename_snapshot(options = {}) raise ArgumentError, 'description is a required parameter' unless options.key? 'description' { - 'task_state' => 'success' + 'task_state' => 'success', } end end diff --git a/app/models/concerns/fog_extensions/vsphere/snapshots/real.rb b/app/models/concerns/fog_extensions/vsphere/snapshots/real.rb index 1985893..f4a10a9 100644 --- a/app/models/concerns/fog_extensions/vsphere/snapshots/real.rb +++ b/app/models/concerns/fog_extensions/vsphere/snapshots/real.rb @@ -17,7 +17,7 @@ def remove_snapshot(options = {}) task.wait_for_completion { - 'task_state' => task.info.state + 'task_state' => task.info.state, } end diff --git a/app/models/foreman_snapshot_management/proxmox_extensions.rb b/app/models/foreman_snapshot_management/proxmox_extensions.rb index c4bfc45..a0ae9aa 100644 --- a/app/models/foreman_snapshot_management/proxmox_extensions.rb +++ b/app/models/foreman_snapshot_management/proxmox_extensions.rb @@ -12,7 +12,7 @@ def capabilities # This method creates a Snapshot with a given name and optional description. def create_snapshot(host, name, description, include_ram = false, _quiesce = false) server = find_vm_by_uuid host.uuid - raise _('Name must contain at least 2 characters starting with alphabet. Valid characters are A-Z a-z 0-9 _') unless /^[A-Za-z][\w]{1,}$/.match?(name) + raise _('Name must contain at least 2 characters starting with alphabet. Valid characters are A-Z a-z 0-9 _') unless /^[A-Za-z]\w{1,}$/.match?(name) include_ram = include_ram ? 1 : 0 snapshot = server.snapshots.create(name: name, vmstate: include_ram) diff --git a/app/models/foreman_snapshot_management/snapshot.rb b/app/models/foreman_snapshot_management/snapshot.rb index e006eb8..c15d3ea 100644 --- a/app/models/foreman_snapshot_management/snapshot.rb +++ b/app/models/foreman_snapshot_management/snapshot.rb @@ -12,20 +12,21 @@ class Snapshot include ActiveModel::ForbiddenAttributesProtection define_model_callbacks :create, :save, :destroy, :revert - attr_accessor :id, :raw_snapshot, :parent, :snapshot_mode - attr_writer :create_time, :quiesce - attr_reader :name, :description, :include_ram, :host_id, :quiesce + attr_accessor :id, :raw_snapshot, :parent, :snapshot_mode, :quiesce + attr_writer :create_time + attr_reader :name, :description, :include_ram, :host_id + define_attribute_methods :name, :description, :include_ram def self.model_name Struct.new(:name, :klass, :singular, :plural, :element, - :human, :collection, :param_key, :i18n_key, :route_key, :singular_route_key).new( - 'ForemanSnapshotManagement::Snapshot', ForemanSnapshotManagement::Snapshot, - 'foreman_snapshot_management_snapshot', 'foreman_snapshot_management_snapshots', - 'snapshot', 'Snapshot', 'foreman_snapshot_management/snapshots', - 'snapshot', :'foreman_snapshot_management/snapshot', 'foreman_snapshot_management_snapshots', - 'foreman_snapshot_management_snapshot' - ) + :human, :collection, :param_key, :i18n_key, :route_key, :singular_route_key).new( + 'ForemanSnapshotManagement::Snapshot', ForemanSnapshotManagement::Snapshot, + 'foreman_snapshot_management_snapshot', 'foreman_snapshot_management_snapshots', + 'snapshot', 'Snapshot', 'foreman_snapshot_management/snapshots', + 'snapshot', :'foreman_snapshot_management/snapshot', 'foreman_snapshot_management_snapshots', + 'foreman_snapshot_management_snapshot' + ) end def self.any? diff --git a/config/routes.rb b/config/routes.rb index 85cce4a..003d6d2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -20,7 +20,7 @@ end end - constraints(host_id: %r{[^\/]+}) do + constraints(host_id: %r{[^/]+}) do resources :hosts, only: [] do resources :snapshots, module: 'foreman_snapshot_management', only: [:index, :create, :destroy, :update] do member do diff --git a/lib/foreman_snapshot_management/engine.rb b/lib/foreman_snapshot_management/engine.rb index 600233c..3f1e77a 100644 --- a/lib/foreman_snapshot_management/engine.rb +++ b/lib/foreman_snapshot_management/engine.rb @@ -21,49 +21,49 @@ class Engine < ::Rails::Engine security_block :foreman_snapshot_management do permission :view_snapshots, { :'foreman_snapshot_management/snapshots' => [:index], - :'api/v2/snapshots' => [:index, :show] + :'api/v2/snapshots' => [:index, :show], }, :resource_type => 'Host' permission :create_snapshots, { :'foreman_snapshot_management/snapshots' => [:create, :select_multiple_host, :create_multiple_host], - :'api/v2/snapshots' => [:create] + :'api/v2/snapshots' => [:create], }, :resource_type => 'Host' permission :edit_snapshots, { :'foreman_snapshot_management/snapshots' => [:update], - :'api/v2/snapshots' => [:update] + :'api/v2/snapshots' => [:update], }, :resource_type => 'Host' permission :destroy_snapshots, { :'foreman_snapshot_management/snapshots' => [:destroy], - :'api/v2/snapshots' => [:destroy] + :'api/v2/snapshots' => [:destroy], }, :resource_type => 'Host' permission :revert_snapshots, { :'foreman_snapshot_management/snapshots' => [:revert], - :'api/v2/snapshots' => [:revert] + :'api/v2/snapshots' => [:revert], }, :resource_type => 'Host' end # Adds roles if they do not exist role 'Snapshot Viewer', - [:view_snapshots], - 'Role granting permission only to view snapshots for hosts' + [:view_snapshots], + 'Role granting permission only to view snapshots for hosts' role 'Snapshot Manager', - [ - :view_snapshots, - :create_snapshots, - :edit_snapshots, - :destroy_snapshots, - :revert_snapshots - ], - 'Role granting permission to manage snapshots for hosts' + [ + :view_snapshots, + :create_snapshots, + :edit_snapshots, + :destroy_snapshots, + :revert_snapshots, + ], + 'Role granting permission to manage snapshots for hosts' extend_page('hosts/show') do |context| context.add_pagelet :main_tabs, - :name => N_('Snapshots'), - :partial => 'hosts/snapshots_tab', - :onlyif => proc { |host| host&.compute_resource&.capable?(:snapshots) } + :name => N_('Snapshots'), + :partial => 'hosts/snapshots_tab', + :onlyif => proc { |host| host&.compute_resource&.capable?(:snapshots) } end end end diff --git a/test/controllers/api/v2/snapshots_test.rb b/test/controllers/api/v2/snapshots_test.rb index 4a060a3..f851472 100644 --- a/test/controllers/api/v2/snapshots_test.rb +++ b/test/controllers/api/v2/snapshots_test.rb @@ -6,7 +6,7 @@ class Api::V2::SnapshotsControllerTest < ActionController::TestCase let(:tax_location) { Location.find_by(name: 'Location 1') } let(:tax_organization) { Organization.find_by(name: 'Organization 1') } let(:compute_resource) do - cr = FactoryBot.create(:compute_resource, :vmware, :uuid => 'Solutions', :locations => [tax_location], organizations: [tax_organization]) + cr = FactoryBot.create(:compute_resource, :vmware, :uuid => 'Solutions', :locations => [tax_location], :organizations => [tax_organization]) ComputeResource.find_by(id: cr.id) end let(:uuid) { '5032c8a5-9c5e-ba7a-3804-832a03e16381' } diff --git a/test/controllers/foreman_snapshot_management/snapshots_controller_test.rb b/test/controllers/foreman_snapshot_management/snapshots_controller_test.rb index 76dbe80..5e6edf6 100644 --- a/test/controllers/foreman_snapshot_management/snapshots_controller_test.rb +++ b/test/controllers/foreman_snapshot_management/snapshots_controller_test.rb @@ -7,7 +7,7 @@ class SnapshotsControllerTest < ActionController::TestCase let(:tax_location) { Location.find_by(name: 'Location 1') } let(:tax_organization) { Organization.find_by(name: 'Organization 1') } let(:compute_resource) do - cr = FactoryBot.create(:compute_resource, :vmware, :uuid => 'Solutions', :locations => [tax_location], organizations: [tax_organization]) + cr = FactoryBot.create(:compute_resource, :vmware, :uuid => 'Solutions', :locations => [tax_location], :organizations => [tax_organization]) ComputeResource.find_by(id: cr.id) end let(:uuid) { '5032c8a5-9c5e-ba7a-3804-832a03e16381' }