Merge pull request #90 from ATIX-AG/fix_tests_and_rubocop

Fix tests and rubocop

.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

.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:

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"

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|

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
 

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

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

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

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
 

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)

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?

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

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

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' }

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' }