Add RuboCop for code style enforcement

.github/workflows/unit_tests.yaml

@@ -11,6 +11,10 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
+  rubocop:
+    name: Rubocop
+    uses: theforeman/actions/.github/workflows/rubocop.yml@v0
+
   test:
     name: Tests
     uses: theforeman/actions/.github/workflows/test-gem.yml@v0

.rubocop.yml

@@ -0,0 +1,42 @@
+require:
+  - rubocop-performance
+  - rubocop-minitest
+
+AllCops:
+  NewCops: enable
+
+Layout/EmptyLineAfterGuardClause:
+  Enabled: false
+
+Layout/LineLength:
+  Enabled: false
+
+Metrics:
+  Enabled: false
+
+Style/FormatStringToken:
+  EnforcedStyle: template
+
+Style/HashSyntax:
+  EnforcedStyle: no_mixed_keys
+
+Style/SymbolArray:
+  EnforcedStyle: brackets
+
+Style/Documentation:
+  Enabled: false
+
+Style/GuardClause:
+  Enabled: false
+
+Style/StringLiterals:
+  Enabled: false
+
+Style/FrozenStringLiteralComment:
+  Enabled: false
+
+Style/IfUnlessModifier:
+  Enabled: false
+
+Gemspec/RequireMFA:
+  Enabled: false

Gemfile

@@ -1,19 +1,23 @@
-source "https://rubygems.org"
+# frozen_string_literal: true
+
+source 'https://rubygems.org'
 
 gemspec
 
 gem 'gettext', '>= 3.1.3', '< 4.0.0'
 gem 'rake', '~> 13.0'
 
 group :test do
-  gem 'thor'
   gem 'minitest', '~> 5.18'
   gem 'minitest-spec-context'
-  gem 'simplecov'
   gem 'mocha'
-  gem 'ci_reporter', '>= 1.6.3', "< 2.0.0", :require => false
+  gem 'rubocop', '~> 1.57.0'
+  gem 'rubocop-minitest', '~> 0.9.0'
+  gem 'rubocop-performance', '~> 1.5.2'
+  gem 'simplecov'
+  gem 'thor'
 end
 
 # load local gemfile
 local_gemfile = File.join(File.dirname(__FILE__), 'Gemfile.local')
-self.instance_eval(Bundler.read_file(local_gemfile)) if File.exist?(local_gemfile)
+instance_eval(Bundler.read_file(local_gemfile)) if File.exist?(local_gemfile)

Rakefile

@@ -1,6 +1,5 @@
 require 'rake/testtask'
 require 'bundler/gem_tasks'
-require 'ci/reporter/rake/minitest'
 
 Rake::TestTask.new do |t|
   t.libs.push "lib"
@@ -17,3 +16,13 @@ require "hammer_cli_foreman_discovery/version"
 require "hammer_cli_foreman_discovery/i18n"
 require "hammer_cli/i18n/find_task"
 HammerCLI::I18n::FindTask.define(HammerCLIForemanDiscovery::I18n::LocaleDomain.new, HammerCLIForemanDiscovery.version.to_s)
+
+begin
+  require 'rubocop/rake_task'
+rescue LoadError
+  # RuboCop is optional
+  task default: :test
+else
+  RuboCop::RakeTask.new
+  task default: [:rubocop, :test]
+end

hammer_cli_foreman_discovery.gemspec

@@ -1,5 +1,4 @@
-# -*- coding: utf-8 -*-
-$:.unshift(File.expand_path('../lib', __FILE__))
+$LOAD_PATH.unshift(File.expand_path('lib', __dir__))
 
 # Maintain your gem's version:
 require "hammer_cli_foreman_discovery/version"
@@ -13,9 +12,9 @@ Gem::Specification.new do |s|
   s.email       = ["ohadlevy@gmail.com"]
   s.homepage    = "https://github.com/theforeman/hammer-cli-foreman-discovery"
   s.summary     = "Foreman CLI plugin for managing discovery hosts in foreman"
-  s.description = <<DESC
-Contains the code for managing host discovery in foreman(results and progress) in the Hammer CLI.
-DESC
+  s.description = <<~DESC
+    Contains the code for managing host discovery in foreman(results and progress) in the Hammer CLI.
+  DESC
 
   s.files = Dir['{lib,locale,config}/**/*', 'LICENSE', 'README*']
   s.extra_rdoc_files = Dir['LICENSE', 'README*']

lib/hammer_cli_foreman_discovery.rb

@@ -5,16 +5,13 @@ module HammerCLIForemanDiscovery
     require 'hammer_cli_foreman_discovery/discovery_references'
 
     HammerCLI::MainCommand.lazy_subcommand('discovery', _("Manipulate discovered hosts."),
-                                           'HammerCLIForemanDiscovery::DiscoveredHost', 'hammer_cli_foreman_discovery/discovery'
-    )
+                                           'HammerCLIForemanDiscovery::DiscoveredHost', 'hammer_cli_foreman_discovery/discovery')
 
     HammerCLI::MainCommand.lazy_subcommand('discovery-rule', _("Manipulate discovered rules."),
-                                           'HammerCLIForemanDiscovery::DiscoveryRule', 'hammer_cli_foreman_discovery/discovery_rule'
-    )
-
-  rescue => e
+                                           'HammerCLIForemanDiscovery::DiscoveryRule', 'hammer_cli_foreman_discovery/discovery_rule')
+  rescue StandardError => e
     handler = HammerCLIForeman::ExceptionHandler.new(:context => {}, :adapter => :base)
     handler.handle_exception(e)
-    raise HammerCLI::ModuleLoadingError.new(e)
+    raise HammerCLI::ModuleLoadingError, e
   end
 end

lib/hammer_cli_foreman_discovery/command_extensions/provision_with_puppet.rb

@@ -18,7 +18,7 @@ class ProvisionWithPuppet < HammerCLI::CommandExtensions
         description: _('Puppet environment. Required if host is managed and value is not inherited from host group'),
         deprecation: _("Use %s instead") % '--puppet-environment[-id]',
         deprecated: { '--environment' => _("Use %s instead") % '--puppet-environment[-id]',
-                      '--environment-id' => _("Use %s instead") % '--puppet-environment[-id]'}
+                      '--environment-id' => _("Use %s instead") % '--puppet-environment[-id]' }
       ) do
         parent '--environment-id', 'ENVIRONMENT_ID', _(''),
                format: HammerCLI::Options::Normalizers::Number.new,
@@ -33,15 +33,13 @@ class ProvisionWithPuppet < HammerCLI::CommandExtensions
         parent '--puppet-proxy-id', 'PUPPET_PROXY_ID', _(''),
                format: HammerCLI::Options::Normalizers::Number.new,
                attribute_name: :option_puppet_proxy_id
-
       end
       option_family(
         aliased_resource: 'puppet_ca_proxy'
       ) do
         parent '--puppet-ca-proxy-id', 'PUPPET_CA_PROXY_ID', _(''),
                format: HammerCLI::Options::Normalizers::Number.new,
                attribute_name: :option_puppet_ca_proxy_id
-
       end
 
       request_params do |params, command_object|

lib/hammer_cli_foreman_discovery/discovery.rb

@@ -4,11 +4,10 @@
 
 module HammerCLIForemanDiscovery
   def self.exception_handler_class
-       HammerCLIForeman::ExceptionHandler
+    HammerCLIForeman::ExceptionHandler
   end
 
   class DiscoveredHost < HammerCLIForeman::Command
-
     resource :discovered_hosts
 
     class ListCommand < HammerCLIForeman::ListCommand
@@ -38,7 +37,6 @@ class InfoCommand < HammerCLIForeman::InfoCommand
       build_options
     end
 
-
     class FactsCommand < HammerCLIForeman::AssociatedResourceListCommand
       command_name "facts"
       resource :fact_values, :index
@@ -77,8 +75,8 @@ class ProvisionCommand < HammerCLIForeman::UpdateCommand
 
       option "--root-password", "ROOT_PW", " "
       option "--ask-root-password", "ASK_ROOT_PW", " ",
-                  :format => HammerCLI::Options::Normalizers::Bool.new
-      bool_format           = {}
+             :format => HammerCLI::Options::Normalizers::Bool.new
+      bool_format = {}
       bool_format[:format] = HammerCLI::Options::Normalizers::Bool.new
       option "--managed", "MANAGED", " ", bool_format
       bool_format[:format] = HammerCLI::Options::Normalizers::Bool.new
@@ -89,14 +87,14 @@ class ProvisionCommand < HammerCLIForeman::UpdateCommand
       option "--overwrite", "OVERWRITE", " ", bool_format
 
       option "--parameters", "PARAMS", _("Host parameters"),
-                  :format => HammerCLI::Options::Normalizers::KeyValueList.new
+             :format => HammerCLI::Options::Normalizers::KeyValueList.new
       option "--interface", "INTERFACE", _("Interface parameters"), :multivalued => true,
-                  :format => HammerCLI::Options::Normalizers::KeyValueList.new
+                                                                    :format => HammerCLI::Options::Normalizers::KeyValueList.new
       option "--provision-method", "METHOD", " ",
-                  :format => HammerCLI::Options::Normalizers::Enum.new(['build', 'image'])
+             :format => HammerCLI::Options::Normalizers::Enum.new(%w[build image])
 
       def ask_password
-        prompt = _("Enter the root password for the host:") + '_'
+        prompt = "#{_('Enter the root password for the host:')}_"
         ask(prompt) { |q| q.echo = false }
       end
 
@@ -119,16 +117,15 @@ def parameter_attributes
         return {} unless option_parameters
         option_parameters.collect do |key, value|
           if value.is_a? String
-            {"name"=>key, "value"=>value}
+            { "name" => key, "value" => value }
           else
-            {"name"=>key, "value"=>value.inspect}
+            { "name" => key, "value" => value.inspect }
           end
         end
       end
 
-      build_options without: %i[
-        root_pass ptable_id host_parameters_attributes
-        puppet_class_ids environment_id puppet_proxy_id puppet_ca_proxy_id
+      build_options without: [
+        :root_pass, :ptable_id, :host_parameters_attributes, :puppet_class_ids, :environment_id, :puppet_proxy_id, :puppet_ca_proxy_id
       ] do |o|
         # TODO: Until the API is cleaned up
         o.expand.except(:environments)
@@ -180,10 +177,10 @@ def execute
             resource.call(:reboot_all, {})
             print_message _("Rebooting hosts")
             HammerCLI::EX_OK
-          rescue RestClient::UnprocessableEntity => error
-            response = JSON.parse(error.response)
-            response = HammerCLIForeman.record_to_common_format(response) unless response.has_key?('message')
-            output.print_error(response['host_details'].map {|i| i['name'] + ": " + i['error'] }.join("\n"))
+          rescue RestClient::UnprocessableEntity => e
+            response = JSON.parse(e.response)
+            response = HammerCLIForeman.record_to_common_format(response) unless response.key?('message')
+            output.print_error(response['host_details'].map { |i| "#{i['name']}: #{i['error']}" }.join("\n"))
             HammerCLI::EX_DATAERR
           end
         else
@@ -208,5 +205,4 @@ class RefreshFactsCommand < HammerCLIForeman::SingleResourceCommand
     end
     autoload_subcommands
   end
-
 end

lib/hammer_cli_foreman_discovery/discovery_references.rb

@@ -1,5 +1,4 @@
 module HammerCLIForemanDiscovery
-
   module DiscoveryReferences
     def self.hosts(dsl)
       dsl.build do
@@ -9,4 +8,4 @@ def self.hosts(dsl)
       end
     end
   end
-end
+end

lib/hammer_cli_foreman_discovery/discovery_rule.rb

@@ -8,21 +8,17 @@ def self.exception_handler_class
   end
 
   module CommonDiscoveryRuleUpdateOptions
-
     def self.included(base)
       base.option "--hosts-limit", "HOSTS_LIMIT", _("Enables to limit maximum amount of provisioned hosts per rule"),
                   :attribute_name => :option_max_count
       base.build_options :without => :max_count
     end
-
   end
 
   class DiscoveryRule < HammerCLIForeman::Command
-
     resource :discovery_rules
 
     class ListCommand < HammerCLIForeman::ListCommand
-
       output do
         field :id, _("ID")
         field :name, _("Name")
@@ -37,7 +33,6 @@ class ListCommand < HammerCLIForeman::ListCommand
     end
 
     class InfoCommand < HammerCLIForeman::InfoCommand
-
       output ListCommand.output_definition do
         field :hostname, _('Hostname template')
         HammerCLIForemanDiscovery::DiscoveryReferences.hosts(self)
@@ -70,5 +65,4 @@ class DeleteCommand < HammerCLIForeman::DeleteCommand
 
     autoload_subcommands
   end
-
 end

lib/hammer_cli_foreman_discovery/i18n.rb

@@ -2,9 +2,7 @@
 
 module HammerCLIForemanDiscovery
   module I18n
-
     class LocaleDomain < HammerCLI::I18n::LocaleDomain
-
       def translated_files
         Dir.glob(File.join(File.dirname(__FILE__), '../**/*.rb'))
       end
@@ -17,8 +15,7 @@ def domain_name
         'hammer_cli_foreman_discovery'
       end
     end
-
   end
 end
 
-HammerCLI::I18n.add_domain(HammerCLIForemanDiscovery::I18n::LocaleDomain.new)
+HammerCLI::I18n.add_domain(HammerCLIForemanDiscovery::I18n::LocaleDomain.new)

test/unit/discovery_resource_mock.rb

@@ -1,9 +1,8 @@
 require File.join(Gem.loaded_specs['hammer_cli_foreman'].full_gem_path, 'test/unit/apipie_resource_mock')
 
 module DiscoveryResourceMock
-
   def self.discovered_hosts_index
-    ResourceMocks.mock_action_call(:discovered_hosts, :index, [ { } ])
+    ResourceMocks.mock_action_call(:discovered_hosts, :index, [{}])
   end
 
   def self.discovered_hosts_show
@@ -15,34 +14,34 @@ def self.discovered_hosts_show
 
   def self.facts_index
     ResourceMocks.mock_action_call(:fact_values, :index, {
-         "total"=>5604,
-         "subtotal"=>0,
-         "page"=>1,
-         "per_page"=>20,
-         "search"=>"",
-         "sort" => {
-             "by" => nil,
-             "order" => nil
-         },
-         "results"=>[{
-                         "some.host.com" => {
-                             "network_br180"=>"10.32.83.0",
-                             "mtu_usb0"=>"1500",
-                             "physicalprocessorcount"=>"1",
-                             "rubyversion"=>"1.8.7"
-                         }
-                     }]
-     })
+                                     "total" => 5604,
+                                     "subtotal" => 0,
+                                     "page" => 1,
+                                     "per_page" => 20,
+                                     "search" => "",
+                                     "sort" => {
+                                       "by" => nil,
+                                       "order" => nil
+                                     },
+                                     "results" => [{
+                                       "some.host.com" => {
+                                         "network_br180" => "10.32.83.0",
+                                         "mtu_usb0" => "1500",
+                                         "physicalprocessorcount" => "1",
+                                         "rubyversion" => "1.8.7"
+                                       }
+                                     }]
+                                   })
   end
 
   def self.discovery_rules_index
-    ResourceMocks.mock_action_call(:discovery_rules, :index, [ { } ])
+    ResourceMocks.mock_action_call(:discovery_rules, :index, [{}])
   end
 
   def self.discovery_rules_show
     ResourceMocks.mock_action_calls(
-        [:discovery_rules, :index, [{ "id" => 2, "name" => "rule_two" }]],
-        [:discovery_rules, :show, { "id" => 2, "name" => "rule_2" }]
+      [:discovery_rules, :index, [{ "id" => 2, "name" => "rule_two" }]],
+      [:discovery_rules, :show, { "id" => 2, "name" => "rule_2" }]
     )
   end
 end