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

Introduce a class to represent the answer file #337

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
26 changes: 26 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,32 @@ As you may have noticed there are several ways how to specify arguments. Here's
* values specified on CLI
* interactive mode arguments

## Answer File Schema

The answer file schema can be described using Puppet types as such:

```
Hash[
String $puppet_class => Hash[
String $parameter => Enum[true, false, Hash[String, Variant[String, Boolean, Integer, Array, Hash]]]
]
]
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'm not sure this makes sense in this format. Or here. What I wanted to do was document the current schema to allow then to document a Version 2 schema. I considered doing this in a comment in the file itself.

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts about using https://yardoc.org/ in the actual classes as well?

Also, it's not really valid. You can't use names and Enum only describes strings. really it's:

Hash[String, Variant[Boolean, Hash[String, Any]]]

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you think to use yarddoc? Just for documenting the method calls or somehow for this schema?

Copy link
Member

Choose a reason for hiding this comment

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

I was trying to model the structure off of https://github.com/theforeman/puppet-pulpcore/blob/master/templates/apache-fragment.epp#L2

That's a struct and that works different. With that you describe what's essentially a hash, but with explicit keys. See https://puppet.com/docs/puppet/6/lang_data_abstract.html#lang_data_abstract_flexible-struct-data-type

You could certainly use a Struct to describe the scenario file (where we have a limited number of fields, each with their own rules, but not the answers file.

How do you think to use yarddoc? Just for documenting the method calls or somehow for this schema?

I'd consider both. In the class documentation you can give describe the structure and give examples while also documenting the individual methods. That said, a short bit in README about the answers file can make sense. I guess it's a bit of both.

```
ehelms marked this conversation as resolved.
Show resolved Hide resolved

An example of each available option:

```
class_a: true
class_b: false
class_c: {}
class_d:
key: value
key2: 'value'
key3: false
key4: 1
key5: ['a', 'b']
```

## Requirements

Kafo is supported with Puppet versions 4.9+, 5 and 6. Puppet may be installed
Expand Down
16 changes: 16 additions & 0 deletions lib/kafo/answer_file.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
require 'kafo/answer_file/v1'

module Kafo
module AnswerFile

def self.load_answers(filename, version)
case version
when 1
AnswerFile::V1.new(filename)
else
raise InvalidAnswerFileVersion
end
end

end
end
39 changes: 39 additions & 0 deletions lib/kafo/answer_file/base.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
require 'yaml'

module Kafo
module AnswerFile
class Base

attr_reader :answers, :filename

def initialize(filename)
@filename = filename
@answers = YAML.load_file(@filename)
validate
end

def save(data, config_header)
FileUtils.touch @filename
File.chmod 0600, @filename
File.open(@filename, 'w') { |f| f.write(config_header + format(YAML.dump(data))) }
end

def puppet_classes
raise NoMethodError
end

def class_enabled?(puppet_class)
raise NoMethodError
end

def parameters_for_class(puppet_class)
raise NoMethodError
end

def validate
raise NoMethodError
end

end
end
end
33 changes: 33 additions & 0 deletions lib/kafo/answer_file/v1.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
require 'kafo/answer_file/base'

module Kafo
module AnswerFile
class V1 < Base

def puppet_classes
@answers.keys.sort
end

def parameters_for_class(puppet_class)
params = @answers[puppet_class]
params.is_a?(Hash) ? params : {}
end

def class_enabled?(puppet_class)
value = @answers[puppet_class.is_a?(String) ? puppet_class : puppet_class.identifier]
!!value || value.is_a?(Hash)
end

def validate
invalid = @answers.reject do |puppet_class, value|
value.is_a?(Hash) || [true, false].include?(value)
end

unless invalid.empty?
fail InvalidAnswerFile, "Answer file at #{@filename} has invalid values for #{invalid.keys.join(', ')}. Please ensure they are either a hash or true/false."
end
end

end
end
end
53 changes: 28 additions & 25 deletions lib/kafo/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
require 'kafo/data_type_parser'
require 'kafo/execution_environment'
require 'kafo/scenario_option'
require 'kafo/answer_file'

module Kafo
class Configuration
Expand Down Expand Up @@ -43,6 +44,7 @@ class Configuration
ScenarioOption::KAFO_MODULES_DIR => nil,
ScenarioOption::CONFIG_HEADER_FILE => nil,
ScenarioOption::DONT_SAVE_ANSWERS => nil,
ScenarioOption::ANSWER_FILE_VERSION => 1,
}

def self.get_scenario_id(filename)
Expand All @@ -55,14 +57,24 @@ def initialize(file, persist = true)
configure_application
@logger = KafoConfigure.logger

@answer_file = app[:answer_file]
begin
@data = load_yaml_file(@answer_file)
@answer_file = AnswerFile.load_answers(app[:answer_file], app[:answer_file_version] || 1)
rescue Errno::ENOENT
puts "No answer file at #{@answer_file} found, can not continue"
KafoConfigure.exit(:no_answer_file)
KafoConfigure.exit(:no_answer_file) do
@logger.error "No answer file found at #{app[:answer_file]}"
end
rescue InvalidAnswerFileVersion
KafoConfigure.exit(:invalid_values) do
@logger.error "Unsupported answer file version"
end
rescue InvalidAnswerFile => error
KafoConfigure.exit(:invalid_values) do
@logger.error(error.message)
end
end

@answers = @answer_file.answers

@config_dir = File.dirname(@config_file)
@scenario_id = Configuration.get_scenario_id(@config_file)
end
Expand Down Expand Up @@ -95,7 +107,7 @@ def configure_application
def app
@app ||= begin
begin
configuration = load_yaml_file(@config_file)
configuration = YAML.load_file(@config_file)
rescue
configuration = {}
end
Expand Down Expand Up @@ -130,7 +142,7 @@ def has_custom_fact?(key)
def modules
@modules ||= begin
register_data_types
@data.keys.map { |mod| PuppetModule.new(mod, configuration: self).parse }.sort
@answer_file.puppet_classes.map { |mod| PuppetModule.new(mod, configuration: self).parse }.sort
end
end

Expand Down Expand Up @@ -231,27 +243,22 @@ class { '::kafo_configure::dump_values':

# if a value is a true we return empty hash because we have no specific options for a
# particular puppet module
def [](key)
value = @data[key]
value.is_a?(Hash) ? value : {}
def [](puppet_class)
@answer_file.parameters_for_class(puppet_class)
end

def module_enabled?(mod)
value = @data[mod.is_a?(String) ? mod : mod.identifier]
!!value || value.is_a?(Hash)
def module_enabled?(puppet_class)
@answer_file.class_enabled?(puppet_class)
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

def store(data, file = nil)
filename = file || answer_file
FileUtils.touch filename
File.chmod 0600, filename
File.open(filename, 'w') { |f| f.write(config_header + format(YAML.dump(data))) }
def store(answers)
@answer_file.save(answers, config_header)
end

def params
Expand Down Expand Up @@ -322,11 +329,11 @@ def answers

def run_migrations
migrations = Kafo::Migrations.new(migrations_dir)
@app, @data = migrations.run(app, answers)
@app, @answers = migrations.run(app, @answers)
if migrations.migrations.count > 0
@modules = nil # force the lazy loaded modules to reload next time they are used
save_configuration(app)
store(answers)
store(@answers)
migrations.store_applied
@logger.notice("#{migrations.migrations.count} migration/s were applied. Updated configuration was saved.")
end
Expand Down Expand Up @@ -376,10 +383,6 @@ def format(data)
data.gsub('!ruby/sym ', ':')
end

def load_yaml_file(filename)
YAML.load_file(filename)
end

# Loads YAML from mixed output, finding the "---" and "..." document start/end delimiters
def load_yaml_from_output(lines)
start = lines.find_index { |l| l.start_with?('---') }
Expand Down
6 changes: 6 additions & 0 deletions lib/kafo/exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,10 @@ class ConditionError < StandardError

class ParserError < StandardError
end

class InvalidAnswerFileVersion < StandardError
end

class InvalidAnswerFile < StandardError
end
end
4 changes: 2 additions & 2 deletions lib/kafo/kafo_configure.rb
Original file line number Diff line number Diff line change
Expand Up @@ -464,9 +464,9 @@ def argument_missing?(value)
!!self.class.declared_options.find { |opt| opt.handles?(value) }
end

def store_params(file = nil)
def store_params
data = Hash[config.modules.map { |mod| [mod.identifier, mod.enabled? ? mod.params_hash : false] }]
config.store(data, file)
config.store(data)
end

def validate_all(logging = true)
Expand Down
3 changes: 3 additions & 0 deletions lib/kafo/scenario_option.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ class ScenarioOption
# Path to answer file, if the file does not exist a $pwd/config/answers.yaml is used as a fallback
ANSWER_FILE = :answer_file

# The version of the answer file schema being used
ANSWER_FILE_VERSION = :answer_file_version

# Enable colors? If you don't touch this, we'll autodetect terminal capabilities
COLORS = :colors
# Color scheme, we support :bright and :dark (first is better for white background, dark for black background)
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/answer_files/v1/basic-answers.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
class_a: true
class_b:
key: value
class_c: {}
class_d: false
4 changes: 4 additions & 0 deletions test/fixtures/answer_files/v1/invalid-answers.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
class_a: 'true'
class_b:
class_c: 1
38 changes: 38 additions & 0 deletions test/kafo/answer_file_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
require 'test_helper'

describe 'Kafo::AnswerFile' do
Copy link
Member

Choose a reason for hiding this comment

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

Generally you actually use the class reference.

Suggested change
describe 'Kafo::AnswerFile' do
describe Kafo::AnswerFile do

describe 'answer file version 1' do
describe 'valid answer file' do
let(:answer_file_path) { 'test/fixtures/answer_files/v1/basic-answers.yaml' }
let(:answer_file) { Kafo::AnswerFile.load_answers(answer_file_path, 1) }

it 'returns the sorted puppet classes' do
_(answer_file.puppet_classes).must_equal(['class_a', 'class_b', 'class_c', 'class_d'])
end

it 'returns the parameters for a class' do
_(answer_file.parameters_for_class('class_b')).must_equal({'key' => 'value'})
end

it 'returns true for a class with a hash' do
_(answer_file.class_enabled?('class_c')).must_equal(true)
end

it 'returns true for a class set to true' do
_(answer_file.class_enabled?('class_a')).must_equal(true)
end

it 'returns false for a class set to false' do
_(answer_file.class_enabled?('class_d')).must_equal(false)
end
end

describe 'invalid answer file' do
let(:answer_file_path) { 'test/fixtures/answer_files/v1/invalid-answers.yaml' }

it 'exits with invalid_answer_file' do
_(Proc.new { Kafo::AnswerFile.load_answers(answer_file_path, 1) } ).must_raise Kafo::InvalidAnswerFile
end
end
end
end
17 changes: 17 additions & 0 deletions test/kafo/configuration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -213,5 +213,22 @@ module Kafo
end
end
end

describe 'invalid answer file' do
let(:answer_file_path) { 'test/fixtures/answer_files/v1/invalid-answers.yaml' }
let(:dummy_logger) { DummyLogger.new }
let(:config_file) { ConfigFileFactory.build('invalid-answer', {:answer_file => answer_file_path}.to_yaml).path }

before do
Kafo::KafoConfigure.logger = dummy_logger
end

it 'exits with invalid_answer_file' do
must_exit_with_code(21) { Kafo::Configuration.new(config_file, false) }

dummy_logger.rewind
_(dummy_logger.error.read).must_match(%r{Answer file at\s.*\shas invalid values for class_a, class_b, class_c. Please ensure they are either a hash or true/false.\n})
end
end
end
end