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.

Sign up for GitHub

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

Cleanup configuration file #340

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Cleanup configuration file #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d478859
Cleanup spacing inside configuration.rb
ehelms Aug 20, 2021
415a0aa
Removed unused @config_dir instance variable in Configuration
ehelms Aug 20, 2021
c72f8f3
Reference self.class in Configuration class
ehelms Aug 20, 2021
0ad51bf
Fix function that fetches a module by name to use identifier
ehelms Aug 20, 2021
6defecf
Pass logger to Configuration class
ehelms Aug 20, 2021
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
2 changes: 1 addition & 1 deletion bin/kafo-export-params
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ module Kafo

def execute
KafoConfigure.logger = Logger.new(STDERR)
c = Configuration.new(config, false)
c = Configuration.new(config, false, logger: KafoConfigure.logger)
KafoConfigure.config = c
if KafoConfigure.config.parser_cache
KafoConfigure.config.parser_cache.force = true if ARGV.include?('--parser-cache')
Expand Down
17 changes: 8 additions & 9 deletions lib/kafo/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ def self.get_scenario_id(filename)
File.basename(filename, '.yaml')
end

def initialize(file, persist = true)
def initialize(file, persist = true, logger:)
@config_file = file
@persist = persist
@persist = persist
configure_application
@logger = KafoConfigure.logger
@logger = logger

@answer_file = app[:answer_file]
begin
Expand All @@ -63,8 +63,7 @@ def initialize(file, persist = true)
KafoConfigure.exit(:no_answer_file)
end

@config_dir = File.dirname(@config_file)
@scenario_id = Configuration.get_scenario_id(@config_file)
@scenario_id = self.class.get_scenario_id(@config_file)
end

def save_configuration(configuration)
Expand Down Expand Up @@ -100,7 +99,7 @@ def app
configuration = {}
end

result = DEFAULT.merge(configuration || {})
result = DEFAULT.merge(configuration || {})
result[:module_dirs] = result[:modules_dir] || result[:module_dirs]
result.delete(:modules_dir)
result
Expand Down Expand Up @@ -135,7 +134,7 @@ def modules
end

def module(name)
modules.find { |m| m.name == name }
modules.find { |m| m.identifier == name }
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a bugfix. Could you provide a bit more info about why it's wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to update the commit message with more information. The most telling is to look at the hook context documentation. Unfortunately, I think name in some areas of code refers to the Puppet naming scheme with :: and in other places it references this internal "name" construct that Kafo creates -- https://github.com/theforeman/kafo/blob/master/lib/kafo/puppet_module.rb#L175

end

def root_dir
Expand Down Expand Up @@ -242,8 +241,8 @@ def module_enabled?(mod)
end

def config_header
files = [app[:config_header_file], File.join(gem_root, '/config/config_header.txt')].compact
file = files.find { |f| File.exist?(f) }
files = [app[:config_header_file], File.join(gem_root, '/config/config_header.txt')].compact
file = files.find { |f| File.exist?(f) }
@config_header ||= file.nil? ? '' : File.read(file)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/kafo/kafo_configure.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ def request_config_reload

def setup_config(conf_file)
self.class.config_file = conf_file
self.class.config = Configuration.new(self.class.config_file)
self.class.config = Configuration.new(self.class.config_file, logger: logger)

if self.class.config.parser_cache
self.class.config.parser_cache.force = true if ARGV.include?('--parser-cache')
Expand Down
2 changes: 1 addition & 1 deletion lib/kafo/scenario_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ def load_and_setup_configuration(config_file)
end

def load_configuration(config_file)
Configuration.new(config_file)
Configuration.new(config_file, logger: @logger)
end

private
Expand Down
11 changes: 6 additions & 5 deletions test/kafo/configuration_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
module Kafo
describe Configuration do
let(:basic_config_file) { ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path }
let(:basic_config) { Kafo::Configuration.new(basic_config_file, false) }
let(:old_config) { Kafo::Configuration.new(basic_config_file, false) }
let(:logger) { Kafo::KafoConfigure.logger }
let(:basic_config) { Kafo::Configuration.new(basic_config_file, false, logger: logger) }
let(:old_config) { Kafo::Configuration.new(basic_config_file, false, logger: logger) }
let(:current_dir) { File.expand_path('.') }

let(:p_foo) { fake_param('foo', 1) }
Expand Down Expand Up @@ -33,7 +34,7 @@ module Kafo
:answer_file => 'test/fixtures/basic_answers.yaml',
:log_dir => log_dir }
config_file = ConfigFileFactory.build('log_dir', cfg.to_yaml).path
config = Kafo::Configuration.new(config_file, false)
config = Kafo::Configuration.new(config_file, false, logger: logger)
assert_equal File.join(log_dir, 'configuration.log'), config.log_file
refute config.log_exists?
File.open(config.log_file, 'w') { |f| f.write 'non-empty-log' }
Expand All @@ -51,14 +52,14 @@ module Kafo
it 'takes modules_dir' do
cfg = { :modules_dir => './my_modules', :answer_file => 'test/fixtures/basic_answers.yaml'}
config_file = ConfigFileFactory.build('modules_dir_single', cfg.to_yaml).path
config = Kafo::Configuration.new(config_file, false)
config = Kafo::Configuration.new(config_file, false, logger: logger)
assert_equal [File.join(current_dir, 'my_modules')], config.module_dirs
end

it 'takes module_dirs' do
cfg = { :module_dirs => ['./my_modules','./their_modules'] , :answer_file => 'test/fixtures/basic_answers.yaml'}
config_file = ConfigFileFactory.build('module_dirs_plural', cfg.to_yaml).path
config = Kafo::Configuration.new(config_file, false)
config = Kafo::Configuration.new(config_file, false, logger: logger)
assert_equal [File.join(current_dir, 'my_modules'), File.join(current_dir, 'their_modules')], config.module_dirs
end
end
Expand Down
2 changes: 1 addition & 1 deletion test/kafo/execution_environment_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module Kafo
describe ExecutionEnvironment do
let(:config_file) { ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path }
let(:config) { Kafo::Configuration.new(config_file, false) }
let(:config) { Kafo::Configuration.new(config_file, false, logger: Kafo::KafoConfigure.logger) }
let(:execution_environment) { ExecutionEnvironment.new(config) }

after { FileUtils.rm_rf(execution_environment.directory) }
Expand Down
2 changes: 1 addition & 1 deletion test/kafo/parser_cache_reader_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ module Kafo

describe "compatibility with writer" do
before do
KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path)
KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path, logger: Kafo::KafoConfigure.logger)
end

let(:parser) { TestParser.new(BASIC_MANIFEST) }
Expand Down
2 changes: 1 addition & 1 deletion test/kafo/puppet_command_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module Kafo
describe PuppetModule do
before do
KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path)
KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path, logger: Kafo::KafoConfigure.logger)
end

let(:pc) { PuppetCommand.new '' }
Expand Down
2 changes: 1 addition & 1 deletion test/kafo/puppet_module_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module Kafo
describe PuppetModule do
before do
KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path)
KafoConfigure.config = Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path, logger: Kafo::KafoConfigure.logger)
end

let(:parser) { TestParser.new(BASIC_MANIFEST) }
Expand Down
6 changes: 3 additions & 3 deletions test/kafo/scenario_manager_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ def create_and_load_scenarios(content, filename='default.yaml')

describe '#confirm_scenario_change' do
let(:basic_config_file) { ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path }
let(:new_config) { Kafo::Configuration.new(basic_config_file, false) }
let(:new_config) { Kafo::Configuration.new(basic_config_file, false, logger: Kafo::KafoConfigure.logger) }

before :all do
@argv = ARGV
Expand Down Expand Up @@ -288,8 +288,8 @@ def create_and_load_scenarios(content, filename='default.yaml')

describe '#print_scenario_diff' do
let(:basic_config_file) { ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path }
let(:new_config) { Kafo::Configuration.new(basic_config_file, false) }
let(:old_config) { Kafo::Configuration.new(basic_config_file, false) }
let(:new_config) { Kafo::Configuration.new(basic_config_file, false, logger: Kafo::KafoConfigure.logger) }
let(:old_config) { Kafo::Configuration.new(basic_config_file, false, logger: Kafo::KafoConfigure.logger) }

let(:p_foo) { fake_param('foo', 1) }
let(:p_bar) { fake_param('bar', 10) }
Expand Down
6 changes: 3 additions & 3 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,10 @@ class testing4(

class Minitest::Spec
before do
Kafo::KafoConfigure.config = Kafo::Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path)
Kafo::KafoConfigure.logger = Kafo::Logger.new
Kafo::KafoConfigure.config = Kafo::Configuration.new(ConfigFileFactory.build('basic', BASIC_CONFIGURATION).path, logger: Kafo::KafoConfigure.logger)
Kafo::KafoConfigure.root_dir = File.dirname(__FILE__)
Kafo::KafoConfigure.exit_handler = Kafo::ExitHandler.new
Kafo::KafoConfigure.logger = Kafo::Logger.new
Kafo::KafoConfigure.module_dirs = ['test/fixtures/modules']
Kafo::Logging.buffer.clear
end
Expand Down Expand Up @@ -204,7 +204,7 @@ def wont_be_on_stdout(output, *args)
end

def fake_module(mod_name, params)
OpenStruct.new( { :class_name => mod_name, :name => mod_name, :enabled? => true, :params => params } ).tap do |m|
OpenStruct.new( { :identifier => mod_name, :class_name => mod_name, :name => mod_name.gsub('::', '_'), :enabled? => true, :params => params } ).tap do |m|
params.each { |p| p.module = m }
end
end
Expand Down