Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RuboCop for code style enforcement #57

Merged
merged 1 commit into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/unit_tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 42 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to deal with this is to add it, but set the value to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That failed the tests...

Enabled: false
14 changes: 9 additions & 5 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -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)
11 changes: 10 additions & 1 deletion Rakefile
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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
9 changes: 4 additions & 5 deletions hammer_cli_foreman_discovery.gemspec
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -13,9 +12,9 @@ Gem::Specification.new do |s|
s.email = ["[email protected]"]
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*']
Expand Down
11 changes: 4 additions & 7 deletions lib/hammer_cli_foreman_discovery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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|
Expand Down
34 changes: 15 additions & 19 deletions lib/hammer_cli_foreman_discovery/discovery.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -38,7 +37,6 @@ class InfoCommand < HammerCLIForeman::InfoCommand
build_options
end


class FactsCommand < HammerCLIForeman::AssociatedResourceListCommand
command_name "facts"
resource :fact_values, :index
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -208,5 +205,4 @@ class RefreshFactsCommand < HammerCLIForeman::SingleResourceCommand
end
autoload_subcommands
end

end
3 changes: 1 addition & 2 deletions lib/hammer_cli_foreman_discovery/discovery_references.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
module HammerCLIForemanDiscovery

module DiscoveryReferences
def self.hosts(dsl)
dsl.build do
Expand All @@ -9,4 +8,4 @@ def self.hosts(dsl)
end
end
end
end
end
6 changes: 0 additions & 6 deletions lib/hammer_cli_foreman_discovery/discovery_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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)
Expand Down Expand Up @@ -70,5 +65,4 @@ class DeleteCommand < HammerCLIForeman::DeleteCommand

autoload_subcommands
end

end
5 changes: 1 addition & 4 deletions lib/hammer_cli_foreman_discovery/i18n.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
45 changes: 22 additions & 23 deletions test/unit/discovery_resource_mock.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Loading