From 6b972797117717151183846ed1395976bc0d1373 Mon Sep 17 00:00:00 2001 From: nofaralfasi Date: Sun, 3 Mar 2024 17:30:11 +0200 Subject: [PATCH] Add RuboCop for code style enforcement --- .github/workflows/unit_tests.yaml | 4 ++ .rubocop.yml | 39 ++++++++++++++++ Gemfile | 14 +++--- Rakefile | 11 ++++- hammer_cli_foreman_discovery.gemspec | 10 ++--- lib/hammer_cli_foreman_discovery.rb | 11 ++--- .../provision_with_puppet.rb | 4 +- lib/hammer_cli_foreman_discovery/discovery.rb | 34 +++++++------- .../discovery_references.rb | 3 +- .../discovery_rule.rb | 6 --- lib/hammer_cli_foreman_discovery/i18n.rb | 5 +-- test/unit/discovery_resource_mock.rb | 45 +++++++++---------- test/unit/discovery_rules_test.rb | 8 ---- test/unit/discovery_test.rb | 11 +---- 14 files changed, 112 insertions(+), 93 deletions(-) create mode 100644 .rubocop.yml diff --git a/.github/workflows/unit_tests.yaml b/.github/workflows/unit_tests.yaml index 3f6c398..1e3b5d6 100644 --- a/.github/workflows/unit_tests.yaml +++ b/.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 diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 0000000..9c4420b --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1,39 @@ +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 diff --git a/Gemfile b/Gemfile index df12e39..8d67365 100644 --- a/Gemfile +++ b/Gemfile @@ -1,4 +1,6 @@ -source "https://rubygems.org" +# frozen_string_literal: true + +source 'https://rubygems.org' gemspec @@ -6,14 +8,16 @@ 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) diff --git a/Rakefile b/Rakefile index 6d6ba9f..1844265 100644 --- a/Rakefile +++ b/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 diff --git a/hammer_cli_foreman_discovery.gemspec b/hammer_cli_foreman_discovery.gemspec index 8ed5f28..5a3bc79 100644 --- a/hammer_cli_foreman_discovery.gemspec +++ b/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 = < 3.10' s.required_ruby_version = '>= 2.7', '< 4' + s.metadata['rubygems_mfa_required'] = 'true' end diff --git a/lib/hammer_cli_foreman_discovery.rb b/lib/hammer_cli_foreman_discovery.rb index 12acc94..1dd7055 100644 --- a/lib/hammer_cli_foreman_discovery.rb +++ b/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 diff --git a/lib/hammer_cli_foreman_discovery/command_extensions/provision_with_puppet.rb b/lib/hammer_cli_foreman_discovery/command_extensions/provision_with_puppet.rb index f77c5d5..9c92f2c 100644 --- a/lib/hammer_cli_foreman_discovery/command_extensions/provision_with_puppet.rb +++ b/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,7 +33,6 @@ 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' @@ -41,7 +40,6 @@ class ProvisionWithPuppet < HammerCLI::CommandExtensions 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| diff --git a/lib/hammer_cli_foreman_discovery/discovery.rb b/lib/hammer_cli_foreman_discovery/discovery.rb index 9f09dfd..1ea25bc 100644 --- a/lib/hammer_cli_foreman_discovery/discovery.rb +++ b/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 diff --git a/lib/hammer_cli_foreman_discovery/discovery_references.rb b/lib/hammer_cli_foreman_discovery/discovery_references.rb index 3a65525..8c28678 100644 --- a/lib/hammer_cli_foreman_discovery/discovery_references.rb +++ b/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 \ No newline at end of file +end diff --git a/lib/hammer_cli_foreman_discovery/discovery_rule.rb b/lib/hammer_cli_foreman_discovery/discovery_rule.rb index 0eb14af..474cced 100644 --- a/lib/hammer_cli_foreman_discovery/discovery_rule.rb +++ b/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 diff --git a/lib/hammer_cli_foreman_discovery/i18n.rb b/lib/hammer_cli_foreman_discovery/i18n.rb index e32971b..031ad1f 100644 --- a/lib/hammer_cli_foreman_discovery/i18n.rb +++ b/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) \ No newline at end of file +HammerCLI::I18n.add_domain(HammerCLIForemanDiscovery::I18n::LocaleDomain.new) diff --git a/test/unit/discovery_resource_mock.rb b/test/unit/discovery_resource_mock.rb index 5525735..0a94cba 100644 --- a/test/unit/discovery_resource_mock.rb +++ b/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 diff --git a/test/unit/discovery_rules_test.rb b/test/unit/discovery_rules_test.rb index 4390e3c..689e7bc 100644 --- a/test/unit/discovery_rules_test.rb +++ b/test/unit/discovery_rules_test.rb @@ -6,7 +6,6 @@ require 'hammer_cli_foreman_discovery/discovery_rule' describe HammerCLIForemanDiscovery::DiscoveryRule do - include CommandTestHelper describe "ListCommand" do @@ -29,7 +28,6 @@ end describe "InfoCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveryRule::InfoCommand.new("", ctx) } before :each do @@ -47,22 +45,18 @@ it_should_print_columns ["ID", "Name", "Priority", "Search", "Host Group", "Hosts Limit", "Enabled", "Hostname template", "Hosts", "Locations", "Organizations"] end end - end describe "DeleteCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveryRule::DeleteCommand.new("", ctx) } describe "parameters" do it_should_accept "name", ["--name=rule"] it_should_accept "id", ["--id=1"] end - end describe "CreateCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveryRule::CreateCommand.new("", ctx) } describe "parameters" do @@ -85,7 +79,6 @@ end describe "UpdateCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveryRule::UpdateCommand.new("", ctx) } describe "parameters" do @@ -101,5 +94,4 @@ end end end - end diff --git a/test/unit/discovery_test.rb b/test/unit/discovery_test.rb index 17e4ba0..050bd1e 100644 --- a/test/unit/discovery_test.rb +++ b/test/unit/discovery_test.rb @@ -6,7 +6,6 @@ require 'hammer_cli_foreman_discovery/discovery' describe HammerCLIForemanDiscovery::DiscoveredHost do - include CommandTestHelper describe "ListCommand" do @@ -29,7 +28,6 @@ end describe "InfoCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveredHost::InfoCommand.new("", ctx) } before :each do @@ -45,21 +43,18 @@ with_params ["--id=1"] do it_should_print_n_records 1 it_should_print_columns ["ID", "Name", "MAC", "Last report", "Subnet", "CPUs", "Memory", "Disk count", "Disks size", "Organization", "Location"] - it_should_print_columns ["IP", "Model"] + it_should_print_columns %w[IP Model] end end - end describe "DeleteCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveredHost::DeleteCommand.new("", ctx) } describe "parameters" do it_should_accept "name", ["--name=host"] it_should_accept "id", ["--id=1"] end - end describe "ProvisionCommand" do @@ -85,14 +80,12 @@ end describe "AutoProvisionCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveredHost::AutoProvisionCommand.new("", ctx) } describe "parameters" do it_should_accept "name", ["--name=host"] it_should_accept "id", ["--id=1"] end - end describe "RebootCommand" do @@ -113,7 +106,6 @@ end describe "FactsCommand" do - let(:cmd) { HammerCLIForemanDiscovery::DiscoveredHost::FactsCommand.new("", ctx) } before(:each) do @@ -132,5 +124,4 @@ end end end - end