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 new configuration option for puppet classes #338

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
32 changes: 24 additions & 8 deletions lib/kafo/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class Configuration
ScenarioOption::KAFO_MODULES_DIR => nil,
ScenarioOption::CONFIG_HEADER_FILE => nil,
ScenarioOption::DONT_SAVE_ANSWERS => nil,
ScenarioOption::CLASSES => {},
Copy link
Member Author

Choose a reason for hiding this comment

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

The standard nomenclature we have been using is modules; but what we really seem to specify everywhere are puppet classes not puppet modules. Thus I decided to aim for using the correct representation.

}

def self.get_scenario_id(filename)
Expand Down Expand Up @@ -130,7 +131,27 @@ def has_custom_fact?(key)
def modules
@modules ||= begin
register_data_types
@data.keys.map { |mod| PuppetModule.new(mod, configuration: self).parse }.sort

@data.map do |name, values|
if (class_config = app[:classes][name.to_sym])
can_disable = class_config[:can_disable] || false
excluded_params = class_config[:exclude] || []
else
can_disable = true
excluded_params = []
end

enabled = !!values || values.is_a?(Hash)

puppet_mod = PuppetModule.new(
name,
configuration: self,
enabled: enabled,
can_disable: can_disable,
excluded_params: excluded_params
)
puppet_mod.parse
end.sort
end
end

Expand Down Expand Up @@ -159,7 +180,7 @@ def kafo_modules_dir
end

def add_module(name)
mod = PuppetModule.new(name, configuration: self).parse
mod = PuppetModule.new(name, configuration: self, enabled: false).parse
unless modules.map(&:name).include?(mod.name)
mod.enable
@modules << mod
Expand Down Expand Up @@ -236,11 +257,6 @@ def [](key)
value.is_a?(Hash) ? value : {}
end

def module_enabled?(mod)
value = @data[mod.is_a?(String) ? mod : mod.identifier]
!!value || value.is_a?(Hash)
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) }
Expand Down Expand Up @@ -295,7 +311,7 @@ def params_changed(old_config)
def params_missing(old_config)
# finds params that are present but will be missing in the new config
old_config.params.select do |par|
next if !par.module.enabled? || !module_enabled?(par.module.name)
next if !par.module.enabled?
param(par.module.class_name, par.name).nil?
end
end
Expand Down
8 changes: 6 additions & 2 deletions lib/kafo/kafo_configure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -388,11 +388,15 @@ def set_options
end

modules.each do |mod|
app_option d("--[no-]enable-#{mod.name}"), :flag, "Enable '#{mod.name}' puppet module",
:default => mod.enabled?
if mod.can_disable?
app_option d("--[no-]enable-#{mod.name}"), :flag, "Enable '#{mod.name}' puppet module (current: #{mod.enabled?})", :default => mod.enabled?
elsif !mod.enabled?
app_option d("--enable-#{mod.name}"), :flag, "Enable '#{mod.name}' puppet module (current: #{mod.enabled?})"
end
Comment on lines +393 to +395
Copy link
Member

Choose a reason for hiding this comment

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

I guess https://github.com/theforeman/kafo/pull/338/files#r701787011 should rather have been here, but 🤷‍♀️

end

params.sort.each do |param|
next if param.module.excluded_param?(dashize(param.name))
doc = param.doc.nil? ? 'UNDOCUMENTED' : param.doc.join("\n")
app_option parametrize(param), '', doc + " (current: #{param.value_to_s})",
:multivalued => param.multivalued?
Expand Down
17 changes: 14 additions & 3 deletions lib/kafo/puppet_module.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def self.find_parser
end
end

def initialize(identifier, parser: nil, configuration: KafoConfigure.config)
def initialize(identifier, parser: nil, configuration: KafoConfigure.config, enabled: nil, can_disable: true, excluded_params: [])
@identifier = identifier
@configuration = configuration
@name = get_name
Expand All @@ -47,14 +47,21 @@ def initialize(identifier, parser: nil, configuration: KafoConfigure.config)
@params_path = get_params_path
@params_class_name = get_params_class_name
@raw_data = nil
@enabled = nil
@enabled = enabled
@can_disable = can_disable
@excluded_params = excluded_params
end

def enabled?
@enabled.nil? ? @enabled = @configuration.module_enabled?(self) : @enabled
@enabled
end

def can_disable?
@can_disable
end

def disable
return unless can_disable?
@enabled = false
end

Expand Down Expand Up @@ -97,6 +104,10 @@ def params_hash
Hash[params.map { |param| [param.name, param.value] }]
end

def excluded_param?(param)
@excluded_params.include?(param)
end

def <=>(other)
@configuration.app[:low_priority_modules].each do |module_name|
return 1 if self.name.include?(module_name) && !other.name.include?(module_name)
Expand Down
2 changes: 2 additions & 0 deletions lib/kafo/scenario_option.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,7 @@ class ScenarioOption
# implements checks to verify this is correct with the Puppet version
# that's running. This can be used to bypass the checks
SKIP_PUPPET_VERSION_CHECK = :skip_puppet_version_check

CLASSES = :classes
end
end
49 changes: 49 additions & 0 deletions test/acceptance/kafo_configure_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -340,5 +340,54 @@ module Kafo
_(stdout).must_include "Hello Kafo\nGoodbye"
end
end

describe 'with classes' do
it 'includes enable flag by default' do
code, out, err = run_command '../bin/kafo-configure --help'
_(code).must_equal 0, err
_(out).must_include "--[no-]enable-testing"
end

it 'does not show disable flag if class cannot be disabled' do
config = YAML.load_file(KAFO_CONFIG)
config[:classes] = {:testing => {:can_disable => false}}
File.open(KAFO_CONFIG, 'w') do |file|
file.write(config.to_yaml)
end

code, out, err = run_command '../bin/kafo-configure --help'
_(code).must_equal 0, err
_(out).wont_include "--[no-]enable-testing"
end

it 'shows enable flag if class is disabled' do
config = YAML.load_file(KAFO_CONFIG)
config[:classes] = {:testing => {:can_disable => false}}
File.open(KAFO_CONFIG, 'w') do |file|
file.write(config.to_yaml)
end

answers = {'testing' => false}
File.open(KAFO_ANSWERS, 'w') do |file|
file.write(answers.to_yaml)
end

code, out, err = run_command '../bin/kafo-configure --help'
_(code).must_equal 0, err
_(out).must_include "--enable-testing"
end
Comment on lines +363 to +378
Copy link
Member

Choose a reason for hiding this comment

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

does that mean that when I've run that-installer --enable-testing once, the next run will error out because --enable-testing is not an option anymore? I can see some (a|se)nsible users cry about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a fair point. My thought was, if the option does nothing, why include it. There are a couple ideas:

  1. If a module is already enabled and cannot be disabled, promote it to --full-help to get it out of the users way but still exist
  2. I have been thinking of a better option for ansible-style users, where users can provide a file as input that maps to installer parameters. That would be a nicer interface for those type of users. Though it may suffer from the same problem (no such option).

Copy link
Member

Choose a reason for hiding this comment

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

Demoting it to full-help sounds good to me.

While we can (and should) make the Ansible life easier by making it understand the installer, we also have users out there who have a 50+ page PDF "how to install/upgrade $project" and they will re-use the same installer parameters on every installer invocation.

Is that (repetition of the params on every invocation) needed? Of course not.
Will the users be totally confused that they have feature X enabled, and yet the installer tells them there is no such thing as feature X (which is how they will read the absence of --enable-X)? Absolutely.
(As me how I know.)


it 'allows declaring parameters that should be excluded as command line options' do
config = YAML.load_file(KAFO_CONFIG)
config[:classes] = {:testing => {:exclude => ['version']}}
File.open(KAFO_CONFIG, 'w') do |file|
file.write(config.to_yaml)
end

code, out, err = run_command '../bin/kafo-configure --full-help'
_(code).must_equal 0, err
_(out).wont_include "--testing-version"
end
end
end
end
6 changes: 2 additions & 4 deletions test/kafo/configuration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,8 @@ module Kafo
describe '#params_missing' do
it 'lists all the params that are missing in the new config' do
basic_config.stub(:modules, [fake_module('mod', [p_foo, p_bar])]) do
basic_config.stub(:module_enabled?, true) do
old_config.stub(:modules, [fake_module('mod', [p_old_foo, p_old_baz])]) do
_(basic_config.params_missing(old_config)).must_equal([p_old_baz])
end
old_config.stub(:modules, [fake_module('mod', [p_old_foo, p_old_baz])]) do
_(basic_config.params_missing(old_config)).must_equal([p_old_baz])
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion test/kafo/puppet_module_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ module Kafo
end

let(:parser) { TestParser.new(BASIC_MANIFEST) }
let(:mod) { PuppetModule.new 'puppet', parser: parser }
let(:mod) { PuppetModule.new 'puppet', parser: parser, enabled: true }

describe "#enabled?" do
specify { _(mod.enabled?).must_equal true }
Expand Down
6 changes: 2 additions & 4 deletions test/kafo/scenario_manager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,8 @@ def create_and_load_scenarios(content, filename='default.yaml')
it 'prints values that will be lost' do
old_config.stub(:modules, [fake_module('mod', [p_old_baz])]) do
new_config.stub(:modules, []) do
new_config.stub(:module_enabled?, true) do
manager.print_scenario_diff(old_config, new_config)
must_be_on_stdout(output, "mod::baz: 100\n")
end
manager.print_scenario_diff(old_config, new_config)
must_be_on_stdout(output, "mod::baz: 100\n")
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion test/kafo/wizard_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module Kafo
let(:output) { StringIO.new }

let(:parser) { @@puppet_parser ||= TestParser.new(BASIC_MANIFEST) }
let(:puppet_module) { PuppetModule.new('puppet', parser: parser).parse }
let(:puppet_module) { PuppetModule.new('puppet', parser: parser, enabled: true).parse }

let(:kafo) do
kafo = OpenStruct.new
Expand Down